Issue #6277 Better handling of exceptions thrown in sessionDestroyed (#…

…6278)

* Issue #6277 Better handling of exceptions thrown in sessionDestroyed

Signed-off-by: Jan Bartel <janb@webtide.com>

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)

jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java

@@ -1972,7 +1972,9 @@ public boolean isPushSupported()
         @Override
         public HttpSession getSession()
         {
-            return new Session(new SessionHandler(), new SessionData(TEST_SESSION_ID,  "", "0.0.0.0", 0, 0, 0, 300));
+            Session session = new Session(new SessionHandler(), new SessionData(TEST_SESSION_ID,  "", "0.0.0.0", 0, 0, 0, 300));
+            session.setResident(true); //necessary for session methods to not throw ISE
+            return session;
         }
 
         @Override

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<String> createdSessions = new ArrayList<>();
     public List<String> 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;
             }
         }
     }

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);
     }

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()