From ecd72596e3ba9f2ee45c3ec696165e70b67d7231 Mon Sep 17 00:00:00 2001 From: jfarcand Date: Mon, 16 Jul 2012 11:10:40 -0400 Subject: [PATCH] Fix for #513 AtmosphereRequest.getSession(create) may return null, even if a session exists. Also fixed the factory to align with other AtmosphereFactory --- .../atmosphere/cpr/AsynchronousProcessor.java | 2 +- .../org/atmosphere/cpr/AtmosphereRequest.java | 15 +++--- .../atmosphere/cpr/AtmosphereResource.java | 6 +++ .../cpr/AtmosphereResourceFactory.java | 50 +++++++++++++++++-- .../cpr/AtmosphereResourceImpl.java | 18 +++++-- .../websocket/DefaultWebSocketProcessor.java | 2 +- .../cpr/AtmosphereResourceFactoryTest.java | 18 +++---- .../java/org/atmosphere/cpr/SessionTest.java | 29 +++++++++++ 8 files changed, 113 insertions(+), 27 deletions(-) diff --git a/modules/cpr/src/main/java/org/atmosphere/cpr/AsynchronousProcessor.java b/modules/cpr/src/main/java/org/atmosphere/cpr/AsynchronousProcessor.java index 66ad6db5839..c19ef7951ca 100755 --- a/modules/cpr/src/main/java/org/atmosphere/cpr/AsynchronousProcessor.java +++ b/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 { diff --git a/modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereRequest.java b/modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereRequest.java index b8eea9a7c3d..3ea9d5f00ae 100644 --- a/modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereRequest.java +++ b/modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereRequest.java @@ -597,12 +597,7 @@ public Map 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); } /** @@ -610,6 +605,14 @@ public HttpSession getSession() { */ @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) { diff --git a/modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereResource.java b/modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereResource.java index 0cc8af1c745..daf10ba1f72 100644 --- a/modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereResource.java +++ b/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); } diff --git a/modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereResourceFactory.java b/modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereResourceFactory.java index 749039b15c0..74591f4a174 100644 --- a/modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereResourceFactory.java +++ b/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 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; + } } diff --git a/modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereResourceImpl.java b/modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereResourceImpl.java index 83bd21ba467..910a81ca95a 100644 --- a/modules/cpr/src/main/java/org/atmosphere/cpr/AtmosphereResourceImpl.java +++ b/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; diff --git a/modules/cpr/src/main/java/org/atmosphere/websocket/DefaultWebSocketProcessor.java b/modules/cpr/src/main/java/org/atmosphere/websocket/DefaultWebSocketProcessor.java index 71a48618cbf..787e41f7391 100644 --- a/modules/cpr/src/main/java/org/atmosphere/websocket/DefaultWebSocketProcessor.java +++ b/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()); diff --git a/modules/cpr/src/test/java/org/atmosphere/cpr/AtmosphereResourceFactoryTest.java b/modules/cpr/src/test/java/org/atmosphere/cpr/AtmosphereResourceFactoryTest.java index ba61e77ee9f..b6a82402afc 100644 --- a/modules/cpr/src/test/java/org/atmosphere/cpr/AtmosphereResourceFactoryTest.java +++ b/modules/cpr/src/test/java/org/atmosphere/cpr/AtmosphereResourceFactoryTest.java @@ -60,7 +60,7 @@ public Enumeration 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())); } } diff --git a/modules/cpr/src/test/java/org/atmosphere/cpr/SessionTest.java b/modules/cpr/src/test/java/org/atmosphere/cpr/SessionTest.java index 3c7571451d4..833071151eb 100644 --- a/modules/cpr/src/test/java/org/atmosphere/cpr/SessionTest.java +++ b/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)); + } }