Issue #14196 fix race in session eviction

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

@@ -309,15 +309,50 @@ public ManagedSession get(String id) throws Exception
      * 
      * @param id The session to retrieve
      * @param enter if true, the usage count of the session will be incremented
-     * @return the session if it exists, null otherwise
+     * @return the session if it exists either in the cache or the store,, null otherwise
      * @throws Exception if the session cannot be loaded
      */
     protected ManagedSession getAndEnter(String id, boolean enter) throws Exception
     {
-        ManagedSession session = null;
-        AtomicReference<Exception> exception = new AtomicReference<Exception>();
+        //As a session may be in the process of being evicted we might need to retry to get it afresh
+        int retries = 10;
 
-        session = doComputeIfAbsent(id, k ->
+        while (retries-- > 0)
+        {
+            ManagedSession session = computeIfAbsent(id);
+
+            if (session == null) //not in cache and doesn't exist in store
+                return null;
+
+            //we have a session, check it
+            try (AutoLock lock = session.lock())
+            {
+                if (session.isResident()) //session is currently in the cache, we can use it
+                {
+                    if (enter)
+                        session.use();
+                    return session;
+                }
+
+                //session is not resident, it might have just been evicted, we should try again
+            }
+        }
+
+        //if we got here we never got a useable session
+        if (LOG.isDebugEnabled())
+            LOG.debug("Retries to get resident session {} exhausted", id);
+        return null;
+    }
+
+    /** Get an existing session object from the cache or load from the session store.
+     * @param id the session to obtain
+     * @return the existing session from the cache or one that has been freshly loaded
+     * @throws Exception if an error occurred
+     */
+    private ManagedSession computeIfAbsent(String id) throws Exception
+    {
+        AtomicReference<Exception> exception = new AtomicReference<Exception>();
+        ManagedSession session = doComputeIfAbsent(id, k ->
         {
             if (LOG.isDebugEnabled())
                 LOG.debug("Session {} not found locally in {}, attempting to load", id, this);
@@ -349,23 +384,6 @@ protected ManagedSession getAndEnter(String id, boolean enter) throws Exception
         Exception ex = exception.get();
         if (ex != null)
             throw ex;
-
-        if (session != null)
-        {
-            try (AutoLock lock = session.lock())
-            {
-                if (!session.isResident()) //session isn't marked as resident in cache
-                {
-                    if (LOG.isDebugEnabled())
-                        LOG.debug("Non-resident session {} in cache", id);
-                    return null;
-                }
-                else if (enter)
-                {
-                    session.use();
-                }
-            }
-        }
 
         return session;
     }

jetty-core/jetty-session/src/test/java/org/eclipse/jetty/session/DefaultSessionCacheTest.java

@@ -16,12 +16,15 @@
 import java.util.Collections;
 import java.util.Random;
 import java.util.Set;
+import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
 
 import org.eclipse.jetty.logging.StacklessLogging;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.server.Session;
 import org.junit.jupiter.api.Test;
+import org.testcontainers.shaded.org.awaitility.Awaitility;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
@@ -440,6 +443,99 @@ public void testRelease()
         assertTrue(store.exists("1234"));
     }
 
+    @Test
+    public void testRaceOnEviction() throws Exception
+    {
+        //Evict if session has not been accessed for 2 seconds or more
+        int evictionSeconds = 2;
+
+        //Use EVICT_ON_INACTIVITY and saveOnInactiveEviction to force a save during eviction
+        Server server = new Server();
+        TestableSessionManager sessionManager = new TestableSessionManager();
+        sessionManager.setServer(server);
+        SessionCacheFactory cacheFactory = newSessionCacheFactory(evictionSeconds, false, true, false, false);
+        DefaultSessionCache cache = (DefaultSessionCache)cacheFactory.getSessionCache(sessionManager);
+
+        final CountDownLatch latch = new CountDownLatch(1);
+        TestableSessionDataStore store = new TestableSessionDataStore()
+        {
+            @Override
+            public void doStore(String id, SessionData data, long lastSaveTime) throws Exception
+            {
+                //wait on a latch before proceeding with save to ensure request is inbound
+                latch.await();
+                super.doStore(id, data, lastSaveTime);
+            }
+        };
+
+        cache.setSessionDataStore(store);
+        sessionManager.setSessionCache(cache);
+        server.addBean(sessionManager);
+        sessionManager.setServer(server);
+        server.start();
+
+        //Make a session that is inactive
+        long now = System.currentTimeMillis();
+        SessionData data = store.newSessionData("1234",
+            now - TimeUnit.MINUTES.toMillis(60),
+            now - TimeUnit.MINUTES.toMillis(40),
+            now - TimeUnit.MINUTES.toMillis(50),
+            0);
+        final ManagedSession session = cache.newSession(data);
+        cache.add("1234", session); //increments usage count
+        session.release(); //decrements the usage count
+
+        assertTrue(session.isResident());
+        assertTrue(session.isValid());
+        assertTrue(session.isIdleLongerThan(evictionSeconds));
+
+        final CountDownLatch evictorRunning = new CountDownLatch(1);
+        //Make a thread to run the checkInactiveSession on it to evict it
+        Thread evictorThread = new Thread(() ->
+        {
+            evictorRunning.countDown();
+            cache.checkInactiveSession(session);
+        });
+        evictorThread.start(); //start thread, will block on the latch
+
+        //Make another thread to call getAndEnter for the same session id
+        AtomicReference<Exception> requestorException = new AtomicReference<Exception>();
+        Thread requestorThread = new Thread(() ->
+        {
+            try
+            {
+                //only proceed if the evictor has started
+                evictorRunning.await();
+                Thread.sleep(500); //small sleep to ensure evictor has begun eviction
+
+                //verify we can get an evicted session back and it is in the cache
+                ManagedSession requestedSession = cache.getAndEnter("1234", true);
+                assertNotNull(requestedSession);
+                assertTrue(requestedSession.isResident());
+                //check original session object is now not resident
+                assertFalse(session.isResident());
+            }
+            catch (Exception e)
+            {
+                requestorException.set(e);
+            }
+        });
+        requestorThread.start();
+
+        //wait a little to ensure requestorThread will be waiting to lock the session
+        //that is being evicted
+        Thread.sleep(1000);
+
+        //Release the latch so the session datastore completes, releases the lock on the
+        //session and the requestorThread can check it isn't resident, then do a reload
+        //of the session data to create a new object
+        latch.countDown();
+
+        //Double check that the cache contains the session, although the pertinent check is
+        //done in the rquestorThread
+        assertTrue(cache.contains("1234"));
+    }
+
     /**
      * Test contains method.
      */