Issue #4920 Restore ability to invalidate sessions on shutdown (#4933)

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

jetty-server/src/main/config/etc/sessions/session-cache-hash.xml

@@ -14,6 +14,7 @@
      <Set name="saveOnCreate"><Property name="jetty.session.saveOnCreate" default="false" /></Set>
      <Set name="removeUnloadableSessions"><Property name="jetty.session.removeUnloadableSessions" default="false"/></Set>
      <Set name="flushOnResponseCommit"><Property name="jetty.session.flushOnResponseCommit" default="false"/></Set>
+     <Set name="invalidateOnShutdown"><Property name="jetty.session.invalidateOnShutdown" default="false"/></Set>
     </New>
    </Arg>
   </Call>

jetty-server/src/main/config/modules/session-cache-hash.mod

@@ -23,3 +23,4 @@ etc/sessions/session-cache-hash.xml
 #jetty.session.saveOnCreate=false
 #jetty.session.removeUnloadableSessions=false
 #jetty.session.flushOnResponseCommit=false
+#jetty.session.invalidateOnShutdown=false

jetty-server/src/main/config/modules/session-cache-null.mod

@@ -18,4 +18,4 @@ etc/sessions/session-cache-null.xml
 [ini-template]
 #jetty.session.saveOnCreate=false
 #jetty.session.removeUnloadableSessions=false
-#jetty.session.flushOnResponseCommit=false
+#jetty.session.flushOnResponseCommit=false

jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCache.java

@@ -99,6 +99,12 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements
      * a dirty session will be flushed to the session store.
      */
     protected boolean _flushOnResponseCommit;
+    
+    /**
+     * If true, when the server shuts down, all sessions in the
+     * cache will be invalidated before being removed.
+     */
+    protected boolean _invalidateOnShutdown;
 
     /**
      * Create a new Session object from pre-existing session data
@@ -815,6 +821,18 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements
         _saveOnInactiveEviction = saveOnEvict;
     }
 
+    @Override
+    public void setInvalidateOnShutdown(boolean invalidateOnShutdown)
+    {
+        _invalidateOnShutdown = invalidateOnShutdown;
+    }
+
+    @Override
+    public boolean isInvalidateOnShutdown()
+    {
+        return _invalidateOnShutdown;
+    }
+
     /**
      * Whether we should save a session that has been inactive before
      * we boot it from the cache.

jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCacheFactory.java

@@ -31,6 +31,19 @@ public abstract class AbstractSessionCacheFactory implements SessionCacheFactory
     boolean _saveOnCreate;
     boolean _removeUnloadableSessions;
     boolean _flushOnResponseCommit;
+    boolean _invalidateOnShutdown;
+    
+    public abstract SessionCache newSessionCache(SessionHandler handler);
+
+    public boolean isInvalidateOnShutdown()
+    {
+        return _invalidateOnShutdown;
+    }
+
+    public void setInvalidateOnShutdown(boolean invalidateOnShutdown)
+    {
+        _invalidateOnShutdown = invalidateOnShutdown;
+    }
 
     /**
      * @return the flushOnResponseCommit
@@ -111,4 +124,17 @@ public abstract class AbstractSessionCacheFactory implements SessionCacheFactory
     {
         _saveOnInactiveEvict = saveOnInactiveEvict;
     }
+
+    @Override
+    public SessionCache getSessionCache(SessionHandler handler)
+    {
+        SessionCache cache = newSessionCache(handler);
+        cache.setEvictionPolicy(getEvictionPolicy());
+        cache.setSaveOnInactiveEviction(isSaveOnInactiveEvict());
+        cache.setSaveOnCreate(isSaveOnCreate());
+        cache.setRemoveUnloadableSessions(isRemoveUnloadableSessions());
+        cache.setFlushOnResponseCommit(isFlushOnResponseCommit());
+        cache.setInvalidateOnShutdown(isInvalidateOnShutdown());
+        return cache;
+    }
 }

jetty-server/src/main/java/org/eclipse/jetty/server/session/DefaultSessionCache.java

@@ -132,29 +132,18 @@ public class DefaultSessionCache extends AbstractSessionCache
     @Override
     public void shutdown()
     {
+        if (LOG.isDebugEnabled())
+            LOG.debug("Shutdown sessions, invalidating = {}", isInvalidateOnShutdown());
+        
         // loop over all the sessions in memory (a few times if necessary to catch sessions that have been
         // added while we're running
         int loop = 100;
+
         while (!_sessions.isEmpty() && loop-- > 0)
         {
             for (Session session : _sessions.values())
             {
-                //if we have a backing store so give the session to it to write out if necessary
+                if (isInvalidateOnShutdown())
-                if (_sessionDataStore != null)
-                {
-                    session.willPassivate();
-                    try
-                    {
-                        _sessionDataStore.store(session.getId(), session.getSessionData());
-                    }
-                    catch (Exception e)
-                    {
-                        LOG.warn(e);
-                    }
-                    doDelete(session.getId()); //remove from memory
-                    session.setResident(false);
-                }
-                else
                 {
                     //not preserving sessions on exit
                     try
@@ -166,6 +155,22 @@ public class DefaultSessionCache extends AbstractSessionCache
                         LOG.ignore(e);
                     }
                 }
+                else
+                {
+                    //write out the session and remove from the cache
+                    if (_sessionDataStore.isPassivating())
+                        session.willPassivate();
+                    try
+                    {
+                        _sessionDataStore.store(session.getId(), session.getSessionData());
+                    }
+                    catch (Exception e)
+                    {
+                        LOG.warn(e);
+                    }
+                    doDelete(session.getId()); //remove from memory
+                    session.setResident(false);
+                }
             }
         }
     }

jetty-server/src/main/java/org/eclipse/jetty/server/session/DefaultSessionCacheFactory.java

@@ -26,14 +26,8 @@ package org.eclipse.jetty.server.session;
 public class DefaultSessionCacheFactory extends AbstractSessionCacheFactory
 {
     @Override
-    public SessionCache getSessionCache(SessionHandler handler)
+    public SessionCache newSessionCache(SessionHandler handler)
     {
-        DefaultSessionCache cache = new DefaultSessionCache(handler);
+        return new DefaultSessionCache(handler);
-        cache.setEvictionPolicy(getEvictionPolicy());
-        cache.setSaveOnInactiveEviction(isSaveOnInactiveEvict());
-        cache.setSaveOnCreate(isSaveOnCreate());
-        cache.setRemoveUnloadableSessions(isRemoveUnloadableSessions());
-        cache.setFlushOnResponseCommit(isFlushOnResponseCommit());
-        return cache;
     }
 }

jetty-server/src/main/java/org/eclipse/jetty/server/session/DefaultSessionIdManager.java

@@ -485,7 +485,9 @@ public class DefaultSessionIdManager extends ContainerLifeCycle implements Sessi
         {
             for (Handler h : tmp)
             {
-                if (h.isStarted())
+                //This method can be called on shutdown when the handlers are STOPPING, so only
+                //check that they are not already stopped
+                if (!h.isStopped() && !h.isFailed())
                     handlers.add((SessionHandler)h);
             }
         }

jetty-server/src/main/java/org/eclipse/jetty/server/session/NullSessionCacheFactory.java

@@ -55,14 +55,23 @@ public class NullSessionCacheFactory extends AbstractSessionCacheFactory
         if (LOG.isDebugEnabled())
             LOG.debug("Ignoring eviction policy setting for NullSessionCaches");
     }
+    
+    @Override
+    public boolean isInvalidateOnShutdown()
+    {
+        return false; //meaningless for NullSessionCache
+    }
 
     @Override
-    public SessionCache getSessionCache(SessionHandler handler)
+    public void setInvalidateOnShutdown(boolean invalidateOnShutdown)
     {
-        NullSessionCache cache = new NullSessionCache(handler);
+        if (LOG.isDebugEnabled())
-        cache.setSaveOnCreate(isSaveOnCreate());
+            LOG.debug("Ignoring invalidateOnShutdown setting for NullSessionCaches");
-        cache.setRemoveUnloadableSessions(isRemoveUnloadableSessions());
+    }
-        cache.setFlushOnResponseCommit(isFlushOnResponseCommit());
+
-        return cache;
+    @Override
+    public SessionCache newSessionCache(SessionHandler handler)
+    {
+        return new NullSessionCache(handler);
     }
 }

jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionCache.java

@@ -312,4 +312,13 @@ public interface SessionCache extends LifeCycle
      * before the response is committed.
      */
     boolean isFlushOnResponseCommit();
+    
+    /**
+     * If true, all existing sessions in the cache will be invalidated when
+     * the server shuts down. Default is false.
+     * @param invalidateOnShutdown
+     */
+    void setInvalidateOnShutdown(boolean invalidateOnShutdown);
+    
+    boolean isInvalidateOnShutdown();
 }

tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/AbstractSessionCacheTest.java

@@ -20,9 +20,7 @@ package org.eclipse.jetty.server.session;
 
 import java.util.Collections;
 import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.TimeUnit;
-
 import javax.servlet.http.HttpSession;
 import javax.servlet.http.HttpSessionActivationListener;
 import javax.servlet.http.HttpSessionEvent;
@@ -119,11 +117,25 @@ public abstract class AbstractSessionCacheTest
             ++activateCalls;
         }
     }
-    
+
-    public abstract AbstractSessionCacheFactory newSessionCacheFactory(int evictionPolicy, boolean saveOnCreate, 
+    public abstract AbstractSessionCacheFactory newSessionCacheFactory(int evictionPolicy,
-                                                                       boolean saveOnInactiveEvict, boolean removeUnloadableSessions,
+                                                                       boolean saveOnCreate, 
+                                                                       boolean saveOnInactiveEvict,
+                                                                       boolean removeUnloadableSessions,
                                                                        boolean flushOnResponseCommit);
 
+    public abstract void checkSessionBeforeShutdown(String id,
+                                                    SessionDataStore store, 
+                                                    SessionCache cache, 
+                                                    TestSessionActivationListener activationListener,
+                                                    TestHttpSessionListener sessionListener) throws Exception;
+    
+    public abstract void checkSessionAfterShutdown(String id,
+                                                   SessionDataStore store,
+                                                   SessionCache cache,
+                                                   TestSessionActivationListener activationListener,
+                                                   TestHttpSessionListener sessionListener) throws Exception;
+
     /**
      * Test that a session that exists in the datastore, but that cannot be
      * read will be invalidated and deleted, and thus a request to re-use that
@@ -243,11 +255,14 @@ public abstract class AbstractSessionCacheTest
         assertEquals(now - 20, session.getCreationTime());
     }
     
+    /**
+     * Test state of session with call to commit
+     * 
+     * @throws Exception
+     */
     @Test
     public void testCommit() throws Exception
     {
-        //Test state of session with call to commit
-        
         Server server = new Server();
 
         ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS);
@@ -320,11 +335,14 @@ public abstract class AbstractSessionCacheTest
         commitAndCheckSaveState(cache, store, session, false, true, false, true, 0, 0);
     }
     
+    /**
+     * Test what happens with various states of a session when commit
+     * is called before release
+     * @throws Exception
+     */
     @Test
     public void testCommitAndRelease() throws Exception
     {
-        //test what happens with various states of a session when commit
-        //is called before release
         Server server = new Server();
 
         ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS);
@@ -421,7 +439,7 @@ public abstract class AbstractSessionCacheTest
         assertFalse(session.getSessionData().isDirty());
         assertTrue(session.getSessionData().isMetaDataDirty());
     }
-    
+
     /**
      * Test the exist method.
      */
@@ -596,6 +614,92 @@ public abstract class AbstractSessionCacheTest
         cache.newSession(null, "1234", now, TimeUnit.MINUTES.toMillis(10));
         assertFalse(store.exists("1234"));
     }
+    
+    /**
+     * Test shutting down the server with invalidateOnShutdown==false
+     * 
+     * @throws Exception
+     */
+    @Test
+    public void testNoInvalidateOnShutdown()
+        throws Exception
+    {
+        Server server = new Server();
+
+        ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS);
+        context.setContextPath("/test");
+        context.setServer(server);
+        server.setHandler(context);
+
+        AbstractSessionCacheFactory cacheFactory = newSessionCacheFactory(SessionCache.NEVER_EVICT, false, false, false, false);
+        SessionCache cache = cacheFactory.getSessionCache(context.getSessionHandler());
+
+        TestSessionDataStore store = new TestSessionDataStore(true);//fake passivation
+        cache.setSessionDataStore(store);
+        context.getSessionHandler().setSessionCache(cache);
+        TestHttpSessionListener sessionListener = new TestHttpSessionListener();
+        context.getSessionHandler().addEventListener(sessionListener);
+
+        server.start();
+
+        //put a session in the cache and store
+        long now = System.currentTimeMillis();
+        SessionData data = store.newSessionData("1234", now - 20, now - 10, now - 20, TimeUnit.MINUTES.toMillis(10));
+        Session session = cache.newSession(data);
+        TestSessionActivationListener activationListener = new TestSessionActivationListener();
+        cache.add("1234", session);
+        session.setAttribute("aaa", activationListener);
+        cache.release("1234", session);
+        checkSessionBeforeShutdown("1234", store, cache, activationListener, sessionListener);
+
+        server.stop(); //calls shutdown
+
+        checkSessionAfterShutdown("1234", store, cache, activationListener, sessionListener);
+    }
+    
+    /**
+     * Test shutdown of the server with invalidateOnShutdown==true
+     * @throws Exception
+     */
+    @Test
+    public void testInvalidateOnShutdown()
+        throws Exception
+    {
+        Server server = new Server();
+
+        ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS);
+        context.setContextPath("/test");
+        server.setHandler(context);
+
+        //flushOnResponseCommit is true
+        AbstractSessionCacheFactory cacheFactory = newSessionCacheFactory(SessionCache.NEVER_EVICT, false, false, false, true);
+        cacheFactory.setInvalidateOnShutdown(true);
+        SessionCache cache = cacheFactory.getSessionCache(context.getSessionHandler());
+
+
+        TestSessionDataStore store = new TestSessionDataStore(true); //fake a passivating store
+        cache.setSessionDataStore(store);
+        context.getSessionHandler().setSessionCache(cache);
+        TestHttpSessionListener sessionListener = new TestHttpSessionListener();
+        context.getSessionHandler().addEventListener(sessionListener);
+
+        server.start();
+
+        //Make a session in the store and cache and check that it is invalidated on shutdown
+        long now = System.currentTimeMillis();
+        SessionData data = store.newSessionData("8888", now - 20, now - 10, now - 20, TimeUnit.MINUTES.toMillis(10));
+        Session session = cache.newSession(data);
+        cache.add("8888", session);
+
+        TestSessionActivationListener activationListener = new TestSessionActivationListener();
+        session.setAttribute("aaa", activationListener);
+        cache.release("8888", session);
+        checkSessionBeforeShutdown("8888", store, cache, activationListener, sessionListener);
+
+        server.stop();
+
+        checkSessionAfterShutdown("8888", store, cache, activationListener, sessionListener);
+    }
 
     public void commitAndCheckSaveState(SessionCache cache, TestSessionDataStore store, Session session, 
                                         boolean expectedBeforeDirty, boolean expectedBeforeMetaDirty, 
@@ -611,7 +715,7 @@ public abstract class AbstractSessionCacheTest
         assertEquals(expectedAfterMetaDirty, session.getSessionData().isMetaDataDirty());
         assertEquals(expectedAfterNumSaves, store._numSaves.get());
     }
-    
+
     public Session createUnExpiredSession(SessionCache cache, SessionDataStore store, String id)
     {
         long now = System.currentTimeMillis();

tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/DefaultSessionCacheTest.java

@@ -20,7 +20,6 @@ package org.eclipse.jetty.server.session;
 
 import java.util.Random;
 import java.util.concurrent.TimeUnit;
-
 import javax.servlet.http.HttpSession;
 
 import org.eclipse.jetty.server.Request;
@@ -54,6 +53,42 @@ public class DefaultSessionCacheTest extends AbstractSessionCacheTest
         factory.setFlushOnResponseCommit(flushOnResponseCommit);
         return factory;
     }
+    
+    @Override
+    public void checkSessionBeforeShutdown(String id,
+                                           SessionDataStore store,
+                                           SessionCache cache,
+                                           TestSessionActivationListener activationListener,
+                                           TestHttpSessionListener sessionListener) throws Exception
+    {
+        assertTrue(store.exists(id));
+        assertTrue(cache.contains(id));
+        assertFalse(sessionListener.destroyedSessions.contains(id));
+        assertEquals(1, activationListener.passivateCalls);
+        assertEquals(1, activationListener.activateCalls);
+    }
+    
+    @Override
+    public void checkSessionAfterShutdown(String id,
+                                          SessionDataStore store,
+                                          SessionCache cache,
+                                          TestSessionActivationListener activationListener,
+                                          TestHttpSessionListener sessionListener) throws Exception
+    {
+        if (cache.isInvalidateOnShutdown())
+        {
+            assertFalse(store.exists(id));
+            assertFalse(cache.contains(id));
+            assertTrue(sessionListener.destroyedSessions.contains(id));
+        }
+        else
+        {
+            assertTrue(store.exists(id));
+            assertFalse(cache.contains(id));
+            assertEquals(2, activationListener.passivateCalls);
+            assertEquals(1, activationListener.activateCalls); //no re-activate on shutdown
+        }
+    }
 
     @Test
     public void testRenewWithInvalidate() throws Exception
@@ -182,25 +217,26 @@ public class DefaultSessionCacheTest extends AbstractSessionCacheTest
     /**
      * Test sessions are saved when shutdown with a store.
      */
-    @Test
+    /*    @Test
-    public void testShutdownWithSessionStore()
+    public void testNoInvalidateOnShutdown()
         throws Exception
     {
         Server server = new Server();
-
+    
         ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS);
         context.setContextPath("/test");
         context.setServer(server);
-
+        server.setHandler(context);
+    
         AbstractSessionCacheFactory cacheFactory = newSessionCacheFactory(SessionCache.NEVER_EVICT, false, false, false, false);
         DefaultSessionCache cache = (DefaultSessionCache)cacheFactory.getSessionCache(context.getSessionHandler());
-
+    
         TestSessionDataStore store = new TestSessionDataStore(true);//fake passivation
         cache.setSessionDataStore(store);
         context.getSessionHandler().setSessionCache(cache);
-
+    
-        context.start();
+        server.start();
-
+    
         //put a session in the cache and store
         long now = System.currentTimeMillis();
         SessionData data = store.newSessionData("1234", now - 20, now - 10, now - 20, TimeUnit.MINUTES.toMillis(10));
@@ -210,17 +246,18 @@ public class DefaultSessionCacheTest extends AbstractSessionCacheTest
         assertTrue(cache.contains("1234"));
         session.setAttribute("aaa", listener);
         cache.release("1234", session);
-
+    
         assertTrue(store.exists("1234"));
         assertTrue(cache.contains("1234"));
-
+    
-        context.stop(); //calls shutdown
+        server.stop(); //calls shutdown
-
+    
         assertTrue(store.exists("1234"));
         assertFalse(cache.contains("1234"));
         assertEquals(2, listener.passivateCalls);
         assertEquals(1, listener.activateCalls);
     }
+    */
 
     /**
      * Test that a session id can be renewed.

tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/NullSessionCacheTest.java

@@ -45,54 +45,33 @@ public class NullSessionCacheTest extends AbstractSessionCacheTest
         factory.setFlushOnResponseCommit(flushOnResponseCommit);
         return factory;
     }
-    
+
-    @Test
+    @Override
-    public void testShutdownWithSessionStore()
+    public void checkSessionBeforeShutdown(String id,
-        throws Exception
+                                           SessionDataStore store,
+                                           SessionCache cache,
+                                           TestSessionActivationListener activationListener,
+                                           TestHttpSessionListener sessionListener) throws Exception
     {
-        Server server = new Server();
+        assertFalse(cache.contains(id)); //NullSessionCache never caches
+        assertTrue(store.exists(id));
+        assertFalse(sessionListener.destroyedSessions.contains(id));
+        assertEquals(1, activationListener.passivateCalls);
+        assertEquals(0, activationListener.activateCalls); //NullSessionCache always evicts on release, so never reactivates
+    }
 
-        ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS);
+    @Override
-        context.setContextPath("/test");
+    public void checkSessionAfterShutdown(String id,
-        context.setServer(server);
+                                          SessionDataStore store,
-
+                                          SessionCache cache,
-        AbstractSessionCacheFactory cacheFactory = newSessionCacheFactory(SessionCache.NEVER_EVICT, false, false, false, false);
+                                          TestSessionActivationListener activationListener,
-        SessionCache cache = cacheFactory.getSessionCache(context.getSessionHandler());
+                                          TestHttpSessionListener sessionListener) throws Exception
-
+    {
-        TestSessionDataStore store = new TestSessionDataStore(true);//fake passivation
+        assertFalse(cache.contains(id)); //NullSessionCache never caches
-        cache.setSessionDataStore(store);
+        assertTrue(store.exists(id)); //NullSessionCache doesn't do anything on shutdown
-        context.getSessionHandler().setSessionCache(cache);
+        assertFalse(sessionListener.destroyedSessions.contains(id)); //NullSessionCache does nothing on shutdown
-
+        assertEquals(1, activationListener.passivateCalls);
-        context.start();
+        assertEquals(0, activationListener.activateCalls); //NullSessionCache always evicts on release, so never reactivates
-
-        //put a session in the cache and store
-        long now = System.currentTimeMillis();
-        SessionData data = store.newSessionData("1234", now - 20, now - 10, now - 20, TimeUnit.MINUTES.toMillis(10));
-        Session session = cache.newSession(data);
-        TestSessionActivationListener listener = new TestSessionActivationListener();
-        cache.add("1234", session);
-        //cache never contains the session
-        assertFalse(cache.contains("1234"));
-        session.setAttribute("aaa", listener);
-        //write session out on release
-        cache.release("1234", session);
-        assertEquals(1, store._numSaves.get());
-        assertEquals(1, listener.passivateCalls);
-        assertEquals(0, listener.activateCalls); //NullSessionCache always evicts on release, so never reactivates
-
-        assertTrue(store.exists("1234"));
-        //cache never contains session
-        assertFalse(cache.contains("1234"));
-
-        context.stop(); //calls shutdown
-
-        //session should still exist in store
-        assertTrue(store.exists("1234"));
-        //cache never contains the session
-        assertFalse(cache.contains("1234"));
-        //shutdown does not save session
-        assertEquals(1, listener.passivateCalls);
-        assertEquals(0, listener.activateCalls);
     }
     
     @Test