Issue #3936 Provide write-through modes for the NullSessionCache (#3984)

* Issue #3936 Provide write-through modes for the NullSessionCache

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

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

@@ -11,6 +11,11 @@
     <New class="org.eclipse.jetty.server.session.NullSessionCacheFactory">
      <Set name="saveOnCreate"><Property name="jetty.session.saveOnCreate" default="false" /></Set>
      <Set name="removeUnloadableSessions"><Property name="jetty.session.removeUnloadableSessions" default="false"/></Set>
+     <Set name="writeThroughMode">
+         <Call class="org.eclipse.jetty.server.session.NullSessionCache$WriteThroughMode" name="valueOf">
+            <Arg><Property name="jetty.session.writeThroughMode" default="ON_EXIT"/></Arg>
+         </Call>
+    </Set>
     </New>
    </Arg>
   </Call>

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

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

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

@@ -126,7 +126,7 @@ public abstract class AbstractSessionDataStore extends ContainerLifeCycle implem
                     LOG.debug("Store: id={}, dirty={}, lsave={}, period={}, elapsed={}", id, data.isDirty(), data.getLastSaved(), savePeriodMs, (System.currentTimeMillis() - lastSave));
 
                 //save session if attribute changed or never been saved or time between saves exceeds threshold
-                if (data.isDirty() || (lastSave <= 0) || ((System.currentTimeMillis() - lastSave) > savePeriodMs))
+                if (data.isDirty() || (lastSave <= 0) || ((System.currentTimeMillis() - lastSave) >= savePeriodMs))
                 {
                     //set the last saved time to now
                     data.setLastSaved(System.currentTimeMillis());

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

@@ -18,7 +18,13 @@
 
 package org.eclipse.jetty.server.session;
 
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
 import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpSessionAttributeListener;
+import javax.servlet.http.HttpSessionBindingEvent;
+
 
 /**
  * NullSessionCache
@@ -30,6 +36,154 @@ import javax.servlet.http.HttpServletRequest;
  */
 public class NullSessionCache extends AbstractSessionCache
 {
+    /**
+     * If the writethrough mode is ALWAYS or NEW, then use an
+     * attribute listener to ascertain when the attribute has changed.
+     *
+     */
+    public class WriteThroughAttributeListener implements HttpSessionAttributeListener
+    {
+        Set<Session> _sessionsBeingWritten = ConcurrentHashMap.newKeySet();
+        
+        @Override
+        public void attributeAdded(HttpSessionBindingEvent event)
+        {   
+            doAttributeChanged(event);
+        }
+
+        @Override
+        public void attributeRemoved(HttpSessionBindingEvent event)
+        {
+            doAttributeChanged(event);
+        }
+
+        @Override
+        public void attributeReplaced(HttpSessionBindingEvent event)
+        {
+            doAttributeChanged(event);
+        }
+
+        private void doAttributeChanged(HttpSessionBindingEvent event)
+        {
+            if (_writeThroughMode == WriteThroughMode.ON_EXIT)
+                return;
+
+            Session session = (Session)event.getSession();
+            
+            SessionDataStore store = getSessionDataStore();
+            
+            if (store == null)
+                return;
+
+            if (_writeThroughMode == WriteThroughMode.ALWAYS
+                || (_writeThroughMode == WriteThroughMode.NEW && session.isNew()))
+            {
+                //ensure that a call to willPassivate doesn't result in a passivation
+                //listener removing an attribute, which would cause this listener to
+                //be called again
+                if (_sessionsBeingWritten.add(session))
+                {
+                    try
+                    {
+                        //should hold the lock on the session, but as sessions are never shared
+                        //with the NullSessionCache, there can be no other thread modifying the 
+                        //same session at the same time (although of course there can be another
+                        //request modifying its copy of the session data, so it is impossible
+                        //to guarantee the order of writes).
+                        if (store.isPassivating())
+                            session.willPassivate();
+                        store.store(session.getId(), session.getSessionData());
+                        if (store.isPassivating())
+                            session.didActivate();
+                    }
+                    catch (Exception e)
+                    {
+                        LOG.warn("Write through of {} failed", e);
+                    }
+                    finally
+                    {
+                        _sessionsBeingWritten.remove(session);
+                    }
+                }
+            }
+        }
+    }
+
+    /**
+     * Defines the circumstances a session will be written to the backing store. 
+     */
+    public enum WriteThroughMode
+    {
+        /**
+         * ALWAYS means write through every attribute change.
+         */
+        ALWAYS,
+        /**
+         * NEW means to write through every attribute change only 
+         * while the session is freshly created, ie its id has not yet been returned to the client
+         */
+        NEW,
+        /**
+         * ON_EXIT means write the session only when the request exits 
+         * (which is the default behaviour of AbstractSessionCache)
+         */
+        ON_EXIT
+    };
+    
+    private WriteThroughMode _writeThroughMode = WriteThroughMode.ON_EXIT;
+    protected WriteThroughAttributeListener _listener = null;
+    
+
+    /**
+     * @return the writeThroughMode
+     */
+    public WriteThroughMode getWriteThroughMode()
+    {
+        return _writeThroughMode;
+    }
+
+
+    /**
+     * @param writeThroughMode the writeThroughMode to set
+     */
+    public void setWriteThroughMode(WriteThroughMode writeThroughMode)
+    {
+        if (getSessionHandler() == null)
+            throw new IllegalStateException ("No SessionHandler");
+        
+        //assume setting null is the same as ON_EXIT
+        if (writeThroughMode == null)
+        {
+            if (_listener != null)
+                getSessionHandler().removeEventListener(_listener);
+            _listener = null;
+            _writeThroughMode = WriteThroughMode.ON_EXIT;
+            return;
+        }
+        
+        switch (writeThroughMode)
+        {
+            case ON_EXIT:
+            {
+                if (_listener != null)
+                    getSessionHandler().removeEventListener(_listener);
+                _listener = null;
+                break;
+            }
+            case NEW:
+            case ALWAYS:
+            {
+                if (_listener == null)
+                {
+                    _listener = new WriteThroughAttributeListener();
+                    getSessionHandler().addEventListener(_listener);
+                }
+                break;
+            }
+        }
+        _writeThroughMode = writeThroughMode;
+    }
+
 
     /**
      * @param handler The SessionHandler related to this SessionCache
@@ -39,6 +193,7 @@ public class NullSessionCache extends AbstractSessionCache
         super(handler);
         super.setEvictionPolicy(EVICT_ON_SESSION_EXIT);
     }
+    
 
     /**
      * @see org.eclipse.jetty.server.session.SessionCache#shutdown()

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

@@ -27,6 +27,23 @@ public class NullSessionCacheFactory implements SessionCacheFactory
 {
     boolean _saveOnCreate;
     boolean _removeUnloadableSessions;
+    NullSessionCache.WriteThroughMode _writeThroughMode;
+
+    /**
+     * @return the writeThroughMode
+     */
+    public NullSessionCache.WriteThroughMode getWriteThroughMode()
+    {
+        return _writeThroughMode;
+    }
+
+    /**
+     * @param writeThroughMode the writeThroughMode to set
+     */
+    public void setWriteThroughMode(NullSessionCache.WriteThroughMode writeThroughMode)
+    {
+        _writeThroughMode = writeThroughMode;
+    }
 
     /**
      * @return the saveOnCreate
@@ -69,6 +86,7 @@ public class NullSessionCacheFactory implements SessionCacheFactory
         NullSessionCache cache = new NullSessionCache(handler);
         cache.setSaveOnCreate(isSaveOnCreate());
         cache.setRemoveUnloadableSessions(isRemoveUnloadableSessions());
+        cache.setWriteThroughMode(_writeThroughMode);
         return cache;
     }
 }

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

@@ -79,8 +79,8 @@ public class TestSessionDataStore extends AbstractSessionDataStore
     @Override
     public void doStore(String id, SessionData data, long lastSaveTime) throws Exception
     {
-        _numSaves.addAndGet(1);
         _map.put(id, data);
+        _numSaves.addAndGet(1);
     }
 
     @Override

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

@@ -18,13 +18,21 @@
 
 package org.eclipse.jetty.server.session;
 
+import java.io.Serializable;
 import java.util.concurrent.TimeUnit;
 
+import javax.servlet.http.HttpSessionActivationListener;
+import javax.servlet.http.HttpSessionEvent;
+
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.servlet.ServletContextHandler;
 import org.junit.jupiter.api.Test;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 /**
@@ -32,9 +40,275 @@ import static org.junit.jupiter.api.Assertions.assertTrue;
  */
 public class NullSessionCacheTest
 {
-    @Test
+    public static class SerializableTestObject implements Serializable, HttpSessionActivationListener
-    public void testEvictOnExit() throws Exception
     {
+        int count;
+        static int passivates = 0;
+        static int activates = 0;
+        
+        public SerializableTestObject(int i)
+        {
+            count = i;
+        }
+        
+        @Override
+        public void sessionWillPassivate(HttpSessionEvent se)
+        {
+            //should never be called, as we are replaced with the
+            //non-serializable object and thus passivate will be called on that
+            ++passivates;
+        }
+
+        @Override
+        public void sessionDidActivate(HttpSessionEvent se)
+        {
+            ++activates;
+            //remove myself, replace with something serializable
+            se.getSession().setAttribute("pv", new TestObject(count));
+        }
+    }
+    
+    
+    
+    public static class TestObject implements HttpSessionActivationListener
+    {
+        int i;
+        static int passivates = 0;
+        static int activates = 0;
+        
+        public TestObject(int j)
+        {
+            i = j;
+        }
+
+        @Override
+        public void sessionWillPassivate(HttpSessionEvent se)
+        {
+            ++passivates;
+            //remove myself, replace with something serializable
+            se.getSession().setAttribute("pv", new SerializableTestObject(i));
+        }
+
+        @Override
+        public void sessionDidActivate(HttpSessionEvent se)
+        {
+            //this should never be called because we replace ourselves during passivation,
+            //so it is the SerializableTestObject that is activated instead
+            ++activates;
+        }
+    }
+    
+    
+    @Test
+    public void testWritesWithPassivation() throws Exception
+    {
+        //Test that a session that is in the process of being saved cannot cause
+        //another save via a passivation listener
+        Server server = new Server();
+
+        ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS);
+        context.setContextPath("/test");
+        context.setServer(server);
+
+        NullSessionCacheFactory cacheFactory = new NullSessionCacheFactory();
+        cacheFactory.setWriteThroughMode(NullSessionCache.WriteThroughMode.ALWAYS);
+        
+        NullSessionCache cache = (NullSessionCache)cacheFactory.getSessionCache(context.getSessionHandler());
+
+        TestSessionDataStore store = new TestSessionDataStore(true); //pretend to passivate
+        cache.setSessionDataStore(store);
+        context.getSessionHandler().setSessionCache(cache);
+        
+        context.start();
+        
+        //make a session
+        long now = System.currentTimeMillis();
+        SessionData data = store.newSessionData("1234", now - 20, now - 10, now - 20, TimeUnit.MINUTES.toMillis(10));
+        data.setExpiry(now + TimeUnit.DAYS.toMillis(1));
+        Session session = cache.newSession(null, data); //mimic a request making a session
+        cache.add("1234", session);
+        //at this point the session should not be saved to the store
+        assertEquals(0, store._numSaves.get());
+        
+        //set an attribute that is not serializable, should cause a save
+        TestObject obj = new TestObject(1);
+        session.setAttribute("pv", obj);
+        assertTrue(cache._listener._sessionsBeingWritten.isEmpty());
+        assertTrue(store.exists("1234"));
+        assertEquals(1, store._numSaves.get());
+        assertEquals(1, TestObject.passivates);
+        assertEquals(0, TestObject.activates);
+        assertEquals(1, SerializableTestObject.activates);
+        assertEquals(0, SerializableTestObject.passivates);
+    }
+    
+    @Test
+    public void testChangeWriteThroughMode() throws Exception
+    {
+        Server server = new Server();
+
+        ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS);
+        context.setContextPath("/test");
+        context.setServer(server);
+
+        NullSessionCacheFactory cacheFactory = new NullSessionCacheFactory();
+
+        NullSessionCache cache = (NullSessionCache)cacheFactory.getSessionCache(context.getSessionHandler());
+
+        TestSessionDataStore store = new TestSessionDataStore();
+        cache.setSessionDataStore(store);
+        context.getSessionHandler().setSessionCache(cache);
+        
+        assertEquals(NullSessionCache.WriteThroughMode.ON_EXIT, cache.getWriteThroughMode());
+        assertNull(cache._listener);
+        
+        //change mode to NEW
+        cache.setWriteThroughMode(NullSessionCache.WriteThroughMode.NEW);
+        assertEquals(NullSessionCache.WriteThroughMode.NEW, cache.getWriteThroughMode());
+        assertNotNull(cache._listener);
+        assertEquals(1, context.getSessionHandler()._sessionAttributeListeners.size());
+        assertTrue(context.getSessionHandler()._sessionAttributeListeners.contains(cache._listener));
+
+        
+        //change mode to ALWAYS from NEW, listener should remain
+        NullSessionCache.WriteThroughAttributeListener old = cache._listener;
+        cache.setWriteThroughMode(NullSessionCache.WriteThroughMode.ALWAYS);
+        assertEquals(NullSessionCache.WriteThroughMode.ALWAYS, cache.getWriteThroughMode());
+        assertNotNull(cache._listener);
+        assertSame(old,cache._listener);
+        assertEquals(1, context.getSessionHandler()._sessionAttributeListeners.size());
+        
+        //check null is same as ON_EXIT
+        cache.setWriteThroughMode(null);
+        assertEquals(NullSessionCache.WriteThroughMode.ON_EXIT, cache.getWriteThroughMode());
+        assertNull(cache._listener);
+        assertEquals(0, context.getSessionHandler()._sessionAttributeListeners.size());
+        
+        //change to ON_EXIT
+        cache.setWriteThroughMode(NullSessionCache.WriteThroughMode.ON_EXIT);
+        assertEquals(NullSessionCache.WriteThroughMode.ON_EXIT, cache.getWriteThroughMode());
+        assertNull(cache._listener);
+        assertEquals(0, context.getSessionHandler()._sessionAttributeListeners.size());
+    }
+    
+    
+    
+    @Test
+    public void testWriteThroughAlways() throws Exception
+    {
+        Server server = new Server();
+
+        ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS);
+        context.setContextPath("/test");
+        context.setServer(server);
+
+        NullSessionCacheFactory cacheFactory = new NullSessionCacheFactory();
+        cacheFactory.setWriteThroughMode(NullSessionCache.WriteThroughMode.ALWAYS);
+
+        NullSessionCache cache = (NullSessionCache)cacheFactory.getSessionCache(context.getSessionHandler());
+
+        TestSessionDataStore store = new TestSessionDataStore();
+        cache.setSessionDataStore(store);
+        context.getSessionHandler().setSessionCache(cache);
+        context.start();
+
+        //make a session
+        long now = System.currentTimeMillis();
+        SessionData data = store.newSessionData("1234", now - 20, now - 10, now - 20, TimeUnit.MINUTES.toMillis(10));
+        data.setExpiry(now + TimeUnit.DAYS.toMillis(1));
+        Session session = cache.newSession(null, data); //mimic a request making a session
+        cache.add("1234", session);
+        //at this point the session should not be saved to the store
+        assertEquals(0, store._numSaves.get());
+        
+        //check each call to set attribute results in a store
+        session.setAttribute("colour", "blue");
+        assertTrue(store.exists("1234"));
+        assertEquals(1, store._numSaves.get());
+        
+        //mimic releasing the session after the request is finished
+        cache.release("1234", session);
+        assertTrue(store.exists("1234"));
+        assertFalse(cache.contains("1234"));
+        assertEquals(2, store._numSaves.get());
+        
+        //simulate a new request using the previously created session
+        //the session should not now be new
+        session = cache.get("1234"); //get the session again
+        session.access(now); //simulate a request
+        session.setAttribute("spin", "left");
+        assertTrue(store.exists("1234"));
+        assertEquals(3, store._numSaves.get());
+        cache.release("1234", session); //finish with the session
+
+        assertFalse(session.isResident());
+    }
+    
+    @Test
+    public void testWriteThroughNew () throws Exception
+    {
+        Server server = new Server();
+
+        ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS);
+        context.setContextPath("/test");
+        context.setServer(server);
+
+        NullSessionCacheFactory cacheFactory = new NullSessionCacheFactory();
+        cacheFactory.setWriteThroughMode(NullSessionCache.WriteThroughMode.NEW);
+
+        NullSessionCache cache = (NullSessionCache)cacheFactory.getSessionCache(context.getSessionHandler());
+
+        TestSessionDataStore store = new TestSessionDataStore();
+        cache.setSessionDataStore(store);
+        context.getSessionHandler().setSessionCache(cache);
+        context.start();
+
+        //make a session
+        long now = System.currentTimeMillis();
+        SessionData data = store.newSessionData("1234", now - 20, now - 10, now - 20, TimeUnit.MINUTES.toMillis(10));
+        data.setExpiry(now + TimeUnit.DAYS.toMillis(1));
+        Session session = cache.newSession(null, data); //mimic a request making a session
+        cache.add("1234", session);
+        //at this point the session should not be saved to the store
+        assertEquals(0, store._numSaves.get());
+        assertTrue(session.isNew());
+        
+        //check each call to set attribute results in a store while the session is new
+        session.setAttribute("colour", "blue");
+        assertTrue(store.exists("1234"));
+        assertEquals(1, store._numSaves.get());
+        session.setAttribute("charge", "positive");
+        assertEquals(2, store._numSaves.get());
+        
+        //mimic releasing the session after the request is finished
+        cache.release("1234", session);
+        assertTrue(store.exists("1234"));
+        assertFalse(cache.contains("1234"));
+        assertEquals(3, store._numSaves.get()); //even if the session isn't dirty, we will save the access time
+
+
+        //simulate a new request using the previously created session
+        //the session should not now be new, so setAttribute should
+        //not result in a save
+        session = cache.get("1234"); //get the session again
+        session.access(now); //simulate a request
+        assertFalse(session.isNew());
+        assertEquals(3, store._numSaves.get());
+        session.setAttribute("spin", "left");
+        assertTrue(store.exists("1234"));
+        assertEquals(3, store._numSaves.get());
+        session.setAttribute("flavor", "charm");
+        assertEquals(3, store._numSaves.get());
+        cache.release("1234", session); //finish with the session
+        assertEquals(4, store._numSaves.get());//release session should write it out
+        assertFalse(session.isResident());  
+    }
+    
+    
+    @Test
+    public void testNotCached() throws Exception
+    {
+        //Test the NullSessionCache never contains the session
         Server server = new Server();
 
         ServletContextHandler context = new ServletContextHandler(ServletContextHandler.SESSIONS);
@@ -67,6 +341,7 @@ public class NullSessionCacheTest
         session = cache.get("1234"); //get the session again
         session.access(now); //simulate a request
         cache.release("1234", session); //finish with the session
+        assertFalse(cache.contains("1234"));
 
         assertFalse(session.isResident());
     }