Issue #4520 Reinstate throw of UnreadableSessionDataException (#4528)

* Issue #4520 Reinstate throw of UnreadableSessionDataException

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

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

@@ -21,6 +21,7 @@ package org.eclipse.jetty.server.session;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.Set;
+import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Function;
 import javax.servlet.http.HttpServletRequest;
 
@@ -334,12 +335,13 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements
      * 
      * @param id The session to retrieve
      * @param enter if true, the usage count of the session will be incremented
-     * @return
+     * @return the session if it exists, null otherwise
-     * @throws Exception
+     * @throws Exception if the session cannot be loaded
      */
     protected Session getAndEnter(String id, boolean enter) throws Exception
     {
         Session session = null;
+        AtomicReference<Exception> exception = new AtomicReference<Exception>();
 
         session = doComputeIfAbsent(id, k ->
         {
@@ -365,11 +367,15 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements
             }
             catch (Exception e)
             {
-                LOG.warn("Error loading session {}", id, e);
+                exception.set(e);
                 return null;
             }
         });
 
+        Exception ex = exception.get();
+        if (ex != null)
+            throw ex;
+            
         if (session != null)
         {
             try (Lock lock = session.lock())
@@ -741,7 +747,8 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements
                 }
                 catch (Exception e)
                 {
-                    LOG.warn("Passivation of idle session {} failed", session.getId(), e);
+                    LOG.warn("Passivation of idle session {} failed", session.getId());
+                    LOG.warn(e);
                 }
             }
         }
@@ -838,7 +845,8 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements
         }
         catch (Exception e)
         {
-            LOG.warn("Save of new session {} failed", id, e);
+            LOG.warn("Save of new session {} failed", id);
+            LOG.warn(e);
         }
         return session;
     }

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

@@ -303,12 +303,13 @@ public class DefaultSessionIdManager extends ContainerLifeCycle implements Sessi
             }
 
             if (LOG.isDebugEnabled())
-                LOG.debug("Checked {}, in use:", id, inUse);
+                LOG.debug("Checked {}, in use: {}", id, inUse);
             return inUse;
         }
         catch (Exception e)
         {
-            LOG.warn("Problem checking if id {} is in use", id, e);
+            LOG.warn("Problem checking if id {} is in use", id);
+            LOG.warn(e);
             return false;
         }
     }

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

@@ -258,7 +258,8 @@ public class FileSessionDataStore extends AbstractSessionDataStore
         }
         catch (NumberFormatException e)
         {
-            LOG.warn("Not valid session filename {}", p.getFileName(), e);
+            LOG.warn("Not valid session filename {}", p.getFileName());
+            LOG.warn(e);
         }
     }
 
@@ -299,7 +300,8 @@ public class FileSessionDataStore extends AbstractSessionDataStore
                 }
                 catch (Exception x)
                 {
-                    LOG.warn("Unable to delete unrestorable file {} for session {}", filename, id, x);
+                    LOG.warn("Unable to delete unrestorable file {} for session {}", filename, id);
+                    LOG.warn(x);
                 }
             }
             throw e;

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

@@ -865,7 +865,8 @@ public class JDBCSessionDataStore extends AbstractSessionDataStore
                         }
                         catch (Exception e)
                         {
-                            LOG.warn("{} Problem checking if potentially expired session {} exists in db", _context.getWorkerName(), k, e);
+                            LOG.warn("{} Problem checking if potentially expired session {} exists in db", _context.getWorkerName(), k);
+                            LOG.warn(e);
                         }
                     }
                 }

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

@@ -88,7 +88,7 @@ public class NullSessionCache extends AbstractSessionCache
     @Override
     public void setEvictionPolicy(int evictionTimeout)
     {
-        LOG.warn("Ignoring eviction setting:" + evictionTimeout);
+        LOG.warn("Ignoring eviction setting: {}", evictionTimeout);
     }
 
     @Override

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

@@ -916,7 +916,8 @@ public class SessionHandler extends ScopedHandler
                     }
                     catch (Exception e)
                     {
-                        LOG.warn("Invalidating session {} found to be expired when requested", id, e);
+                        LOG.warn("Invalidating session {} found to be expired when requested", id);
+                        LOG.warn(e);
                     }
 
                     return null;
@@ -928,6 +929,7 @@ public class SessionHandler extends ScopedHandler
         }
         catch (UnreadableSessionDataException e)
         {
+            LOG.warn("Error loading session {}", id);
             LOG.warn(e);
             try
             {
@@ -936,7 +938,8 @@ public class SessionHandler extends ScopedHandler
             }
             catch (Exception x)
             {
-                LOG.warn("Error cross-context invalidating unreadable session {}", id, x);
+                LOG.warn("Error cross-context invalidating unreadable session {}", id);
+                LOG.warn(x);
             }
             return null;
         }
@@ -1211,7 +1214,7 @@ public class SessionHandler extends ScopedHandler
                         }
                         catch (Exception e)
                         {
-                            LOG.warn("Session listener threw exception", e);
+                            LOG.warn(e);
                         }
                         //call the attribute removed listeners and finally mark it as invalid
                         session.finishInvalidate();

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

@@ -20,19 +20,25 @@ 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;
 
+import org.eclipse.jetty.server.Request;
 import org.eclipse.jetty.server.Server;
 import org.eclipse.jetty.servlet.ServletContextHandler;
+import org.eclipse.jetty.util.log.Log;
+import org.eclipse.jetty.util.log.StacklessLogging;
 import org.junit.jupiter.api.Test;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 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;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -43,6 +49,58 @@ import static org.junit.jupiter.api.Assertions.assertTrue;
  */
 public abstract class AbstractSessionCacheTest
 {
+    public static class UnreadableSessionDataStore extends AbstractSessionDataStore
+    {
+        int _count;
+        int _calls;
+        SessionData _data;
+
+        public UnreadableSessionDataStore(int count, SessionData data)
+        {
+            _count = count;
+            _data = data;
+        }
+
+        @Override
+        public boolean isPassivating()
+        {
+            return false;
+        }
+
+        @Override
+        public boolean exists(String id) throws Exception
+        {
+            return _data != null;
+        }
+
+        @Override
+        public boolean delete(String id) throws Exception
+        {
+            _data = null;
+            return true;
+        }
+
+        @Override
+        public void doStore(String id, SessionData data, long lastSaveTime) throws Exception
+        {
+        }
+
+        @Override
+        public SessionData doLoad(String id) throws Exception
+        {
+            ++_calls;
+            if (_calls <= _count)
+                throw new UnreadableSessionDataException(id, _context, new IllegalStateException("Throw for test"));
+            else
+                return _data;
+        }
+
+        @Override
+        public Set<String> doGetExpired(Set<String> candidates)
+        {
+            return null;
+        }
+    }
 
     public static class TestSessionActivationListener implements HttpSessionActivationListener
     {
@@ -66,6 +124,57 @@ public abstract class AbstractSessionCacheTest
                                                                        boolean saveOnInactiveEvict, boolean removeUnloadableSessions,
                                                                        boolean flushOnResponseCommit);
 
+    /**
+     * 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
+     * same id will not succeed.
+     * 
+     * @throws Exception
+     */
+    @Test
+    public void testUnreadableSession() throws Exception
+    {
+        Server server = new Server();
+        server.setSessionIdManager(new DefaultSessionIdManager(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());
+
+        //prefill the datastore with a session that will be treated as unreadable
+        UnreadableSessionDataStore store = new UnreadableSessionDataStore(1, new SessionData("1234", "/test", "0.0.0.0", System.currentTimeMillis(), 0,0, -1));
+        cache.setSessionDataStore(store);
+        context.getSessionHandler().setSessionCache(cache);
+        server.start();
+
+        try (StacklessLogging stackless = new StacklessLogging(Log.getLogger("org.eclipse.jetty.server.session")))
+        {
+            //check that session 1234 cannot be read, ie returns null AND
+            //that it is deleted in the datastore
+            Session session = context.getSessionHandler().getSession("1234");
+            assertNull(session);
+            assertFalse(store.exists("1234"));
+
+            //now try to make a session with the same id as if from a request with
+            //a SESSION_ID cookie set - the id from the cookie should not be able to
+            //be re-used because we just deleted the session with that id. Ids cannot
+            //be re-used (unless another context is already using that same id (ie cross
+            //context dispatch), which is not the case in this test).
+            Request request = new Request(null, null);
+            request.setRequestedSessionId("1234");
+            HttpSession newSession = context.getSessionHandler().newHttpSession(request);
+            assertNotEquals("1234", newSession.getId());
+        }
+        finally
+        {
+            server.stop();
+        }
+    }
+
     /**
      * Test that a new Session object can be created from
      * previously persisted data (SessionData).

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

@@ -32,6 +32,8 @@ import org.eclipse.jetty.client.api.ContentResponse;
 import org.eclipse.jetty.client.api.Request;
 import org.eclipse.jetty.servlet.ServletContextHandler;
 import org.eclipse.jetty.servlet.ServletHolder;
+import org.eclipse.jetty.util.log.Log;
+import org.eclipse.jetty.util.log.StacklessLogging;
 import org.junit.jupiter.api.Test;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -177,7 +179,7 @@ public class SessionEvictionFailureTest
             int port1 = server.getPort();
             HttpClient client = new HttpClient();
             client.start();
-            try
+            try (StacklessLogging stackless = new StacklessLogging(Log.getLogger("org.eclipse.jetty.server.session")))
             {
                 String url = "http://localhost:" + port1 + contextPath + servletMapping;