From 91d9850d64076cad97611a3379775e01acddf986 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Sun, 16 May 2021 09:45:50 +1000 Subject: [PATCH] Issue #6277 Better handling of exceptions thrown in sessionDestroyed (#6278) * Issue #6277 Better handling of exceptions thrown in sessionDestroyed Signed-off-by: Jan Bartel --- .../eclipse/jetty/server/session/Session.java | 9 +- .../session/TestHttpSessionListener.java | 26 ++++-- .../server/session/SessionListenerTest.java | 82 +++++++++++++++++-- 3 files changed, 102 insertions(+), 15 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 a34bc0f..ecaf7c7 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 @@ -506,6 +506,7 @@ public class Session implements SessionHandler.SessionIf { try (Lock lock = _lock.lock()) { + checkValidForRead(); return _sessionData.getLastAccessed(); } } @@ -972,14 +973,18 @@ public class Session implements SessionHandler.SessionIf // 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 770627b..b736fdf 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 @@ -34,17 +34,19 @@ 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 accessAttribute = false; + 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) @@ -58,9 +60,21 @@ public class TestHttpSessionListener implements HttpSessionListener } catch (Exception e) { - ex = e; + attributeException = e; } } + + if (lastAccessTime) + { + try + { + se.getSession().getLastAccessedTime(); + } + catch (Exception e) + { + accessTimeException = e; + } + } } public void sessionCreated(HttpSessionEvent 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 ba83986..24ac045 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 @@ -21,6 +21,7 @@ package org.eclipse.jetty.server.session; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.isIn; 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; @@ -74,7 +75,7 @@ public class SessionListenerTest 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); @@ -120,6 +121,73 @@ public class SessionListenerTest } + + /** + * 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. * @@ -144,7 +212,7 @@ public class SessionListenerTest ServletHolder holder = new ServletHolder(servlet); ServletContextHandler context = server1.addContext(contextPath); context.addServlet(holder, servletMapping); - TestHttpSessionListener listener = new TestHttpSessionListener(true); + TestHttpSessionListener listener = new TestHttpSessionListener(true.true); context.getSessionHandler().addEventListener(listener); server1.start(); @@ -171,7 +239,8 @@ public class SessionListenerTest assertThat(sessionId, isIn(listener.destroyedSessions)); - assertNull(listener.ex); + assertNull(listener.attributeException); + assertNull(listener.accessTimeException); } finally { @@ -203,7 +272,7 @@ public class SessionListenerTest 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); @@ -236,7 +305,8 @@ public class SessionListenerTest assertTrue (listener.destroyedSessions.contains("1234")); - assertNull(listener.ex); + assertNull(listener.attributeException); + assertNull(listener.accessTimeException); } finally @@ -245,8 +315,6 @@ public class SessionListenerTest } } - - public static class MySessionBindingListener implements HttpSessionBindingListener, Serializable { private static final long serialVersionUID = 1L; -- 2.23.0