From b926efba1f5a66db2ff966d97e483088841a1e37 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Sat, 15 May 2021 08:18:51 +1000 Subject: [PATCH] Issue #6277 Better handling of exceptions thrown in sessionDestroyed Signed-off-by: Jan Bartel --- .../eclipse/jetty/server/session/Session.java | 13 +-- .../session/TestHttpSessionListener.java | 24 ++++- ...tHttpSessionListenerWithWebappClasses.java | 6 +- .../server/session/SessionListenerTest.java | 95 ++++++++++++++++++- 4 files changed, 119 insertions(+), 19 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java index 2ed4f7f07010..db4dd10b0b40 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/Session.java @@ -469,10 +469,7 @@ public long getLastAccessedTime() { try (AutoLock l = _lock.lock()) { - if (isInvalid()) - { - throw new IllegalStateException("Session not valid"); - } + checkValidForRead(); return _sessionData.getLastAccessed(); } } @@ -867,14 +864,18 @@ public void invalidate() // do the invalidation _handler.callSessionDestroyedListeners(this); } + catch (Exception e) + { + LOG.warn("Error during Session destroy listener", e); + } finally { // call the attribute removed listeners and finally mark it // as invalid finishInvalidate(); + // tell id mgr to remove sessions with same id from all contexts + _handler.getSessionIdManager().invalidateAll(_sessionData.getId()); } - // tell id mgr to remove sessions with same id from all contexts - _handler.getSessionIdManager().invalidateAll(_sessionData.getId()); } } catch (Exception e) diff --git a/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/TestHttpSessionListener.java b/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/TestHttpSessionListener.java index 3982236a3fd3..d733469ce814 100644 --- a/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/TestHttpSessionListener.java +++ b/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/TestHttpSessionListener.java @@ -26,16 +26,18 @@ public class TestHttpSessionListener implements HttpSessionListener public List createdSessions = new ArrayList<>(); public List destroyedSessions = new ArrayList<>(); public boolean accessAttribute = false; - public Exception ex = null; + public boolean lastAccessTime = false; + public Exception attributeException = null; + public Exception accessTimeException = null; - public TestHttpSessionListener(boolean access) + public TestHttpSessionListener(boolean accessAttribute, boolean lastAccessTime) { - accessAttribute = access; + this.accessAttribute = accessAttribute; + this.lastAccessTime = lastAccessTime; } public TestHttpSessionListener() { - accessAttribute = false; } public void sessionDestroyed(HttpSessionEvent se) @@ -49,7 +51,19 @@ public void sessionDestroyed(HttpSessionEvent se) } catch (Exception e) { - ex = e; + attributeException = e; + } + } + + if (lastAccessTime) + { + try + { + se.getSession().getLastAccessedTime(); + } + catch (Exception e) + { + accessTimeException = e; } } } diff --git a/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/TestHttpSessionListenerWithWebappClasses.java b/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/TestHttpSessionListenerWithWebappClasses.java index 598a5ca6c779..5c0488b40c69 100644 --- a/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/TestHttpSessionListenerWithWebappClasses.java +++ b/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/TestHttpSessionListenerWithWebappClasses.java @@ -30,9 +30,9 @@ public TestHttpSessionListenerWithWebappClasses() super(); } - public TestHttpSessionListenerWithWebappClasses(boolean access) + public TestHttpSessionListenerWithWebappClasses(boolean attribute, boolean lastAccessTime) { - super(access); + super(attribute, lastAccessTime); } @Override @@ -47,7 +47,7 @@ public void sessionDestroyed(HttpSessionEvent se) } catch (Exception cnfe) { - ex = cnfe; + attributeException = cnfe; } super.sessionDestroyed(se); } diff --git a/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/SessionListenerTest.java b/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/SessionListenerTest.java index 9fdf911e16be..74287cff0af3 100644 --- a/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/SessionListenerTest.java +++ b/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/SessionListenerTest.java @@ -53,6 +53,7 @@ import static org.hamcrest.Matchers.in; import static org.hamcrest.Matchers.is; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; @@ -87,7 +88,7 @@ public void testListenerWithInvalidation() throws Exception TestServer server = new TestServer(0, inactivePeriod, scavengePeriod, cacheFactory, storeFactory); ServletContextHandler context = server.addContext(contextPath); - TestHttpSessionListener listener = new TestHttpSessionListener(true); + TestHttpSessionListener listener = new TestHttpSessionListener(true, true); context.getSessionHandler().addEventListener(listener); TestServlet servlet = new TestServlet(); ServletHolder holder = new ServletHolder(servlet); @@ -131,6 +132,72 @@ public void testListenerWithInvalidation() throws Exception LifeCycle.stop(server); } } + + /** + * Test that if a session listener throws an exception during sessionDestroyed the session is still invalidated + */ + @Test + public void testListenerWithInvalidationException() throws Exception + { + String contextPath = ""; + String servletMapping = "/server"; + int inactivePeriod = 6; + int scavengePeriod = -1; + + DefaultSessionCacheFactory cacheFactory = new DefaultSessionCacheFactory(); + cacheFactory.setEvictionPolicy(SessionCache.NEVER_EVICT); + TestSessionDataStoreFactory storeFactory = new TestSessionDataStoreFactory(); + storeFactory.setGracePeriodSec(scavengePeriod); + + TestServer server = new TestServer(0, inactivePeriod, scavengePeriod, + cacheFactory, storeFactory); + ServletContextHandler context = server.addContext(contextPath); + ThrowingSessionListener listener = new ThrowingSessionListener(); + context.getSessionHandler().addEventListener(listener); + TestServlet servlet = new TestServlet(); + ServletHolder holder = new ServletHolder(servlet); + context.addServlet(holder, servletMapping); + + try + { + server.start(); + int port1 = server.getPort(); + + HttpClient client = new HttpClient(); + client.start(); + try + { + String url = "http://localhost:" + port1 + contextPath + servletMapping; + // Create the session + ContentResponse response1 = client.GET(url + "?action=init"); + assertEquals(HttpServletResponse.SC_OK, response1.getStatus()); + String sessionCookie = response1.getHeaders().get("Set-Cookie"); + assertNotNull(sessionCookie); + assertTrue(TestServlet.bindingListener.bound); + + String sessionId = TestServer.extractSessionId(sessionCookie); + + // Make a request which will invalidate the existing session + Request request2 = client.newRequest(url + "?action=test"); + ContentResponse response2 = request2.send(); + assertEquals(HttpServletResponse.SC_OK, response2.getStatus()); + + assertTrue(TestServlet.bindingListener.unbound); + + //check session no longer exists + assertFalse(context.getSessionHandler().getSessionCache().contains(sessionId)); + assertFalse(context.getSessionHandler().getSessionCache().getSessionDataStore().exists(sessionId)); + } + finally + { + LifeCycle.stop(client); + } + } + finally + { + LifeCycle.stop(server); + } + } /** * Test that listeners are called when a session expires @@ -172,7 +239,7 @@ public void testSessionExpiresWithListener() throws Exception ServletContextHandler context = server1.addContext(contextPath); context.setClassLoader(contextClassLoader); context.addServlet(holder, servletMapping); - TestHttpSessionListener listener = new TestHttpSessionListenerWithWebappClasses(true); + TestHttpSessionListener listener = new TestHttpSessionListenerWithWebappClasses(true, true); context.getSessionHandler().addEventListener(listener); try @@ -201,7 +268,8 @@ public void testSessionExpiresWithListener() throws Exception assertThat(sessionId, is(in(listener.destroyedSessions))); - assertNull(listener.ex); + assertNull(listener.attributeException); + assertNull(listener.accessTimeException); } finally { @@ -236,7 +304,7 @@ public void testExpiredSession() throws Exception ServletHolder holder = new ServletHolder(servlet); ServletContextHandler context = server1.addContext(contextPath); context.addServlet(holder, servletMapping); - TestHttpSessionListener listener = new TestHttpSessionListener(); + TestHttpSessionListener listener = new TestHttpSessionListener(true, true); context.getSessionHandler().addEventListener(listener); @@ -271,7 +339,8 @@ public void testExpiredSession() throws Exception assertTrue(listener.destroyedSessions.contains("1234")); - assertNull(listener.ex); + assertNull(listener.attributeException); + assertNull(listener.accessTimeException); } finally { @@ -296,6 +365,22 @@ public void sessionDestroyed(HttpSessionEvent se) { } } + + public static class ThrowingSessionListener implements HttpSessionListener + { + + @Override + public void sessionCreated(HttpSessionEvent se) + { + } + + @Override + public void sessionDestroyed(HttpSessionEvent se) + { + throw new IllegalStateException("Exception during sessionDestroyed"); + } + + } @Test public void testSessionListeners()