Fix for #513 AtmosphereRequest.getSession(create) may return null, ev…

…en if a session exists. Also fixed the factory to align with other AtmosphereFactory

modules/cpr/src/main/java/org/atmosphere/cpr/AsynchronousProcessor.java

@@ -217,7 +217,7 @@ Action action(AtmosphereRequest req, AtmosphereResponse res) throws IOException,
         if (resource == null) {
             // TODO: cast is dangerous
             resource = (AtmosphereResourceImpl)
-                    AtmosphereResourceFactory.create(config, handlerWrapper.broadcaster, res, this, handlerWrapper.atmosphereHandler);
+                    AtmosphereResourceFactory.getDefault().create(config, handlerWrapper.broadcaster, res, this, handlerWrapper.atmosphereHandler);
         } else {
             // TODO: This piece of code can be removed, but for backward compat with existing extension we needs it for now.
             try {

modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereRequest.java

@@ -597,19 +597,22 @@ public Map<String, Object> attributes() {
      */
     @Override
     public HttpSession getSession() {
-        HttpSession session = getSession(true);
-        // The underlying request has been recycled, let's return the original value
-        if (session == null) {
-            session = resource().session();
-        }
-        return session;
+        return getSession(true);
     }
 
     /**
      * {@inheritDoc}
      */
     @Override
     public HttpSession getSession(boolean create) {
+        if (resource() != null) {
+            // UGLY, but we need to prevent looping here.
+            HttpSession session = AtmosphereResourceImpl.class.cast(resource()).session;
+            if (session != null) {
+                return session;
+            }
+        }
+
         try {
             return b.request.getSession(create);
         } catch (java.lang.IllegalStateException ex) {

modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereResource.java

@@ -351,4 +351,10 @@ enum TRANSPORT {POLLING, LONG_POLLING, STREAMING, WEBSOCKET, JSONP, UNDEFINED, S
      * @return the {@link HttpSession} is supported, null if not
      */
     HttpSession session();
+
+     /**
+     * Return the {@link HttpSession} is supported, and creates it if not already created.
+     * @return the {@link HttpSession} is supported, and creates it if not already crea
+     */
+    HttpSession session(boolean create);
 }

modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereResourceFactory.java

@@ -28,6 +28,8 @@
  */
 public final class AtmosphereResourceFactory {
 
+    private final static AtmosphereResourceFactory factory = new AtmosphereResourceFactory();
+
     private final static AtmosphereHandler voidAtmosphereHandler = new AbstractReflectorAtmosphereHandler() {
         @Override
         public void onRequest(AtmosphereResource resource) throws IOException {
@@ -38,6 +40,40 @@ public void destroy() {
         }
     };
 
+    /**
+     * Create an {@link AtmosphereResourceImpl}
+     *
+     * @param config   an {@link AtmosphereConfig}
+     * @param request an {@link AtmosphereResponse}
+     * @param a        {@link AsyncSupport}
+     * @return an {@link AtmosphereResourceImpl}
+     */
+    public final AtmosphereResource create(AtmosphereConfig config,
+                                                  AtmosphereRequest request,
+                                                  AtmosphereResponse response,
+                                                  AsyncSupport<?> a) {
+        return new AtmosphereResourceImpl(config, null, request, response, a, voidAtmosphereHandler);
+    }
+
+    /**
+     * Create an {@link AtmosphereResourceImpl}
+     *
+     * @param config      an {@link AtmosphereConfig}
+     * @param broadcaster a {@link Broadcaster}
+     * @param response    an {@link AtmosphereResponse}
+     * @param a           {@link AsyncSupport}
+     * @param handler     an {@link AtmosphereHandler}
+     * @return an {@link AtmosphereResourceImpl}
+     */
+    public final AtmosphereResource create(AtmosphereConfig config,
+                                                  Broadcaster broadcaster,
+                                                  AtmosphereRequest request,
+                                                  AtmosphereResponse response,
+                                                  AsyncSupport<?> a,
+                                                  AtmosphereHandler handler) {
+        return new AtmosphereResourceImpl(config, broadcaster, request, response, a, handler);
+    }
+
     /**
      * Create an {@link AtmosphereResourceImpl}
      *
@@ -48,12 +84,12 @@ public void destroy() {
      * @param handler     an {@link AtmosphereHandler}
      * @return an {@link AtmosphereResourceImpl}
      */
-    public final static AtmosphereResource create(AtmosphereConfig config,
+    public final AtmosphereResource create(AtmosphereConfig config,
                                                   Broadcaster broadcaster,
                                                   AtmosphereResponse response,
                                                   AsyncSupport<?> a,
                                                   AtmosphereHandler handler) {
-        return new AtmosphereResourceImpl(config, broadcaster, response.request(), response, a, handler);
+        return create(config, broadcaster, response.request(), response, a, handler);
     }
 
     /**
@@ -64,7 +100,7 @@ public final static AtmosphereResource create(AtmosphereConfig config,
      * @param a        {@link AsyncSupport}
      * @return an {@link AtmosphereResourceImpl}
      */
-    public final static AtmosphereResource create(AtmosphereConfig config,
+    public final AtmosphereResource create(AtmosphereConfig config,
                                                   AtmosphereResponse response,
                                                   AsyncSupport<?> a) {
         return new AtmosphereResourceImpl(config, null, response.request(), response, a, voidAtmosphereHandler);
@@ -76,7 +112,7 @@ public final static AtmosphereResource create(AtmosphereConfig config,
      * @param uuid the {@link org.atmosphere.cpr.AtmosphereResource#uuid()}
      * @return the {@link AtmosphereResource}, or null if not found.
      */
-    public final static AtmosphereResource remove(String uuid) {
+    public final AtmosphereResource remove(String uuid) {
         AtmosphereResource r = find(uuid);
         if (r != null) {
             BroadcasterFactory.getDefault().removeAllAtmosphereResource(r);
@@ -90,7 +126,7 @@ public final static AtmosphereResource remove(String uuid) {
      * @param uuid the {@link org.atmosphere.cpr.AtmosphereResource#uuid()}
      * @return the {@link AtmosphereResource}, or null if not found.
      */
-    public final static AtmosphereResource find(String uuid) {
+    public final AtmosphereResource find(String uuid) {
         Collection<Broadcaster> l = BroadcasterFactory.getDefault().lookupAll();
         for (Broadcaster b : l) {
             for (AtmosphereResource r : b.getAtmosphereResources()) {
@@ -101,4 +137,8 @@ public final static AtmosphereResource find(String uuid) {
         }
         return null;
     }
+
+    public final static AtmosphereResourceFactory getDefault() {
+        return factory;
+    }
 }

modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereResourceImpl.java

@@ -115,7 +115,7 @@ public class AtmosphereResourceImpl implements AtmosphereResource {
     private final boolean writeHeaders;
     private String padding;
     private final String uuid;
-    private HttpSession session;
+    protected HttpSession session;
 
     /**
      * Create an {@link AtmosphereResource}.
@@ -155,7 +155,7 @@ public AtmosphereResourceImpl(AtmosphereConfig config, Broadcaster broadcaster,
         req.setAttribute(ApplicationConfig.STREAMING_PADDING_MODE, padding);
 
         String s = response.getHeader(HeaderConfig.X_ATMOSPHERE_TRACKING_ID);
-        uuid = s == null? UUID.randomUUID().toString() : s;
+        uuid = s == null ? UUID.randomUUID().toString() : s;
 
         if (config.isSupportSession()) {
             //Keep a reference to an HttpSession in case the associated request get recycled by the underlying container.
@@ -905,14 +905,22 @@ public AtmosphereResourceImpl disableSuspend(boolean disableSuspend) {
      * {@inheritDoc}
      */
     @Override
-    public HttpSession session(){
-        if (session == null && config.isSupportSession()) {
+    public HttpSession session(boolean create) {
+        if (session == null) {
             // http://java.net/jira/browse/GLASSFISH-18856
-            session = req.getSession(true);
+            session = req.getSession(create);
         }
         return session;
     }
 
+    /**
+     * {@inheritDoc}
+     */
+    @Override
+    public HttpSession session() {
+        return session(true);
+    }
+
     public AtmosphereResourceImpl session(HttpSession session) {
         this.session = session;
         return this;

modules/cpr/src/main/java/org/atmosphere/websocket/DefaultWebSocketProcessor.java

@@ -104,7 +104,7 @@ public final void open(final AtmosphereRequest request) throws IOException {
         AtmosphereResponse wsr = new AtmosphereResponse(webSocket, request, destroyable);
         request.headers(configureHeader(request)).setAttribute(WebSocket.WEBSOCKET_SUSPEND, true);
 
-        AtmosphereResource r = AtmosphereResourceFactory.create(framework.getAtmosphereConfig(),
+        AtmosphereResource r = AtmosphereResourceFactory.getDefault().create(framework.getAtmosphereConfig(),
                 wsr,
                 framework.getAsyncSupport());
 

modules/cpr/src/test/java/org/atmosphere/cpr/AtmosphereResourceFactoryTest.java

@@ -60,7 +60,7 @@ public Enumeration<String> getInitParameterNames() {
 
     @Test
     public void createTest() {
-        AtmosphereResource r = AtmosphereResourceFactory.create(mock(AtmosphereConfig.class), mock(Broadcaster.class), AtmosphereResponse.create().request(AtmosphereRequest.create()),
+        AtmosphereResource r = AtmosphereResourceFactory.getDefault().create(mock(AtmosphereConfig.class), mock(Broadcaster.class), AtmosphereResponse.create().request(AtmosphereRequest.create()),
                 mock(AsyncSupport.class), mock(AtmosphereHandler.class));
         assertNotNull(r);
     }
@@ -69,13 +69,13 @@ public void createTest() {
     public void findTest() {
         Broadcaster b1 = BroadcasterFactory.getDefault().get("b1");
         Broadcaster b2 = BroadcasterFactory.getDefault().get("b2");
-        AtmosphereResource r = AtmosphereResourceFactory.create(mock(AtmosphereConfig.class), b1, AtmosphereResponse.create().request(AtmosphereRequest.create()),
+        AtmosphereResource r = AtmosphereResourceFactory.getDefault().create(mock(AtmosphereConfig.class), b1, AtmosphereResponse.create().request(AtmosphereRequest.create()),
                 mock(AsyncSupport.class), mock(AtmosphereHandler.class));
         assertNotNull(r);
 
         b2.addAtmosphereResource(r);
 
-        assertNotNull(AtmosphereResourceFactory.find(r.uuid()));
+        assertNotNull(AtmosphereResourceFactory.getDefault().find(r.uuid()));
 
     }
 
@@ -84,10 +84,10 @@ public void notFoundTest() {
         for (int i = 0; i < 10; i++) {
             BroadcasterFactory.getDefault().get(String.valueOf(i));
         }
-        AtmosphereResource r = AtmosphereResourceFactory.create(mock(AtmosphereConfig.class), BroadcasterFactory.getDefault().lookup("1"), AtmosphereResponse.create().request(AtmosphereRequest.create()),
+        AtmosphereResource r = AtmosphereResourceFactory.getDefault().create(mock(AtmosphereConfig.class), BroadcasterFactory.getDefault().lookup("1"), AtmosphereResponse.create().request(AtmosphereRequest.create()),
                 mock(AsyncSupport.class), mock(AtmosphereHandler.class));
         assertNotNull(r);
-        assertNull(AtmosphereResourceFactory.find(r.uuid()));
+        assertNull(AtmosphereResourceFactory.getDefault().find(r.uuid()));
     }
 
     @Test
@@ -96,13 +96,13 @@ public void deleteTest() {
             BroadcasterFactory.getDefault().get(String.valueOf(i));
         }
         Broadcaster b2 = BroadcasterFactory.getDefault().get("b2");
-        AtmosphereResource r = AtmosphereResourceFactory.create(mock(AtmosphereConfig.class), BroadcasterFactory.getDefault().lookup("1"), AtmosphereResponse.create().request(AtmosphereRequest.create()),
+        AtmosphereResource r = AtmosphereResourceFactory.getDefault().create(mock(AtmosphereConfig.class), BroadcasterFactory.getDefault().lookup("1"), AtmosphereResponse.create().request(AtmosphereRequest.create()),
                 mock(AsyncSupport.class), mock(AtmosphereHandler.class));
         assertNotNull(r);
         b2.addAtmosphereResource(r);
 
-        assertNotNull(AtmosphereResourceFactory.find(r.uuid()));
-        assertEquals(AtmosphereResourceFactory.remove(r.uuid()), r);
-        assertNull(AtmosphereResourceFactory.find(r.uuid()));
+        assertNotNull(AtmosphereResourceFactory.getDefault().find(r.uuid()));
+        assertEquals(AtmosphereResourceFactory.getDefault().remove(r.uuid()), r);
+        assertNull(AtmosphereResourceFactory.getDefault().find(r.uuid()));
     }
 }

modules/cpr/src/test/java/org/atmosphere/cpr/SessionTest.java

@@ -15,13 +15,16 @@
  */
 package org.atmosphere.cpr;
 
+import org.atmosphere.container.BlockingIOCometSupport;
 import org.atmosphere.util.FakeHttpSession;
 import org.testng.annotations.Test;
 
+import javax.servlet.ServletConfig;
 import javax.servlet.ServletException;
 import java.io.IOException;
 import java.util.concurrent.ExecutionException;
 
+import static org.mockito.Mockito.mock;
 import static org.testng.Assert.assertNotNull;
 import static org.testng.Assert.assertNull;
 
@@ -40,4 +43,30 @@ public void basicSessionTest() throws IOException, ServletException, ExecutionEx
         assertNotNull(request.getSession());
         assertNotNull(request.getSession(true));
     }
+
+    @Test
+    public void basicAtmosphereResourceSessionTest() throws IOException, ServletException, ExecutionException, InterruptedException {
+        AtmosphereRequest request = new AtmosphereRequest.Builder().build();
+        AtmosphereResponse response = new AtmosphereResponse.Builder().build();
+
+        AtmosphereResource r = AtmosphereResourceFactory.getDefault().create(new AtmosphereFramework().getAtmosphereConfig(),
+                request,
+                response,
+                mock(AsyncSupport.class));
+
+        assertNull(r.session(false));
+        assertNotNull(r.session());
+        assertNotNull(r.session(true));
+        assertNotNull(r.session());
+
+        request = new AtmosphereRequest.Builder().session(new FakeHttpSession("-1", null, System.currentTimeMillis(), -1)).build();
+        response = new AtmosphereResponse.Builder().build();
+        r = AtmosphereResourceFactory.getDefault().create(new AtmosphereFramework().getAtmosphereConfig(),
+                request,
+                response,
+                mock(AsyncSupport.class));
+
+        assertNotNull(r.session());
+        assertNotNull(r.session(true));
+    }
 }