diff --git a/jetty-memcached/jetty-memcached-sessions/src/test/java/org/eclipse/jetty/memcached/session/TestMemcachedSessions.java b/jetty-memcached/jetty-memcached-sessions/src/test/java/org/eclipse/jetty/memcached/session/TestMemcachedSessions.java index ffc3d8d68f8..01988effeb1 100644 --- a/jetty-memcached/jetty-memcached-sessions/src/test/java/org/eclipse/jetty/memcached/session/TestMemcachedSessions.java +++ b/jetty-memcached/jetty-memcached-sessions/src/test/java/org/eclipse/jetty/memcached/session/TestMemcachedSessions.java @@ -20,6 +20,7 @@ package org.eclipse.jetty.memcached.session; import java.io.IOException; import java.io.PrintWriter; + import javax.servlet.ServletException; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; @@ -31,12 +32,9 @@ import org.eclipse.jetty.client.api.ContentResponse; import org.eclipse.jetty.client.api.Request; import org.eclipse.jetty.server.NetworkConnector; import org.eclipse.jetty.server.Server; -import org.eclipse.jetty.server.session.AbstractSessionCache; import org.eclipse.jetty.server.session.CachingSessionDataStore; +import org.eclipse.jetty.server.session.NullSessionCache; import org.eclipse.jetty.server.session.NullSessionDataStore; -import org.eclipse.jetty.server.session.Session; -import org.eclipse.jetty.server.session.SessionData; -import org.eclipse.jetty.server.session.SessionHandler; import org.eclipse.jetty.servlet.ServletContextHandler; import org.eclipse.jetty.servlet.ServletHolder; import org.junit.jupiter.api.Test; @@ -88,56 +86,6 @@ public class TestMemcachedSessions } } - public static class NullSessionCache extends AbstractSessionCache - { - - public NullSessionCache(SessionHandler handler) - { - super(handler); - } - - @Override - public void shutdown() - { - } - - @Override - public Session newSession(SessionData data) - { - return new Session(_handler, data); - } - - @Override - public Session newSession(HttpServletRequest request, SessionData data) - { - return new Session(_handler, request, data); - } - - @Override - public Session doGet(String id) - { - return null; - } - - @Override - public Session doPutIfAbsent(String id, Session session) - { - return null; - } - - @Override - public boolean doReplace(String id, Session oldValue, Session newValue) - { - return true; - } - - @Override - public Session doDelete(String id) - { - return null; - } - } - @Test public void testMemcached() throws Exception { diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCache.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCache.java index 48ccac95481..821485b98f8 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCache.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/AbstractSessionCache.java @@ -21,6 +21,8 @@ package org.eclipse.jetty.server.session; import java.util.Collections; import java.util.HashSet; import java.util.Set; +import java.util.function.Function; + import javax.servlet.http.HttpServletRequest; import org.eclipse.jetty.util.StringUtil; @@ -133,6 +135,18 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements * @return null if the session wasn't already in the map, or the existing entry otherwise */ protected abstract Session doPutIfAbsent(String id, Session session); + + /** + * Compute the mappingFunction to create a Session object iff the session + * with the given id isn't already in the map, otherwise return the existing Session. + * This method is expected to have precisely the same behaviour as + * {@link java.util.concurrent.ConcurrentHashMap#computeIfAbsent} + * + * @param id the session id + * @param mappingFunction the function to load the data for the session + * @return an existing Session from the cache + */ + protected abstract Session doComputeIfAbsent(String id, Function mappingFunction); /** * Replace the mapping from id to oldValue with newValue @@ -152,22 +166,6 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements */ public abstract Session doDelete(String id); - /** - * PlaceHolder - */ - protected class PlaceHolderSession extends Session - { - - /** - * @param handler SessionHandler to which this session belongs - * @param data the session data - */ - public PlaceHolderSession(SessionHandler handler, SessionData data) - { - super(handler, data); - } - } - /** * @param handler the {@link SessionHandler} to use */ @@ -343,113 +341,53 @@ public abstract class AbstractSessionCache extends ContainerLifeCycle implements protected Session getAndEnter(String id, boolean enter) throws Exception { Session session = null; - Exception ex = null; - while (true) + session = doComputeIfAbsent(id, k -> { - session = doGet(id); + if (LOG.isDebugEnabled()) + LOG.debug("Session {} not found locally in {}, attempting to load", id, this); - if (_sessionDataStore == null) - break; //can't load any session data so just return null or the session object - - if (session == null) + try { - if (LOG.isDebugEnabled()) - LOG.debug("Session {} not found locally in {}, attempting to load", id, this); - - //didn't get a session, try and create one and put in a placeholder for it - PlaceHolderSession phs = new PlaceHolderSession(_handler, new SessionData(id, null, null, 0, 0, 0, 0)); - Lock phsLock = phs.lock(); - Session s = doPutIfAbsent(id, phs); - if (s == null) + Session s = loadSession(k); + if (s != null) { - //My placeholder won, go ahead and load the full session data - try + try (Lock lock = s.lock()) { - session = loadSession(id); - if (session == null) - { - //session does not exist, remove the placeholder - doDelete(id); - phsLock.close(); - break; - } - - try (Lock lock = session.lock()) - { - //swap it in instead of the placeholder - boolean success = doReplace(id, phs, session); - if (!success) - { - //something has gone wrong, it should have been our placeholder - doDelete(id); - session = null; - LOG.warn("Replacement of placeholder for session {} failed", id); - phsLock.close(); - break; - } - else - { - //successfully swapped in the session - session.setResident(true); - if (enter) - session.use(); - phsLock.close(); - break; - } - } - } - catch (Exception e) - { - ex = e; //remember a problem happened loading the session - doDelete(id); //remove the placeholder - phsLock.close(); - session = null; - break; + s.setResident(true); //ensure freshly loaded session is resident } } else { - //my placeholder didn't win, check the session returned - phsLock.close(); - try (Lock lock = s.lock()) - { - //is it a placeholder? or is a non-resident session? In both cases, chuck it away and start again - if (!s.isResident() || s instanceof PlaceHolderSession) - { - session = null; - continue; - } - //I will use this session too - session = s; - if (enter) - session.use(); - break; - } + if (LOG.isDebugEnabled()) + LOG.debug("Session {} not loaded by store", id); } + return s; } - else + catch (Exception e) { - //check the session returned - try (Lock lock = session.lock()) - { - //is it a placeholder? or is it passivated? In both cases, chuck it away and start again - if (!session.isResident() || session instanceof PlaceHolderSession) - { - session = null; - continue; - } + LOG.warn("Error loading session {}", id, e); + return null; + } + }); - //got the session - if (enter) - session.use(); - break; + if (session != null) + { + try (Lock 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(); } } } - if (ex != null) - throw ex; return session; } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/DefaultSessionCache.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/DefaultSessionCache.java index ef00dd46edf..1cc782bcb4d 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/DefaultSessionCache.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/DefaultSessionCache.java @@ -19,6 +19,8 @@ package org.eclipse.jetty.server.session; import java.util.concurrent.ConcurrentHashMap; +import java.util.function.Function; + import javax.servlet.http.HttpServletRequest; import org.eclipse.jetty.util.annotation.ManagedAttribute; @@ -80,18 +82,12 @@ public class DefaultSessionCache extends AbstractSessionCache return _stats.getTotal(); } - /** - * - */ @ManagedOperation(value = "reset statistics", impact = "ACTION") public void resetStats() { _stats.reset(); } - /** - * @see org.eclipse.jetty.server.session.AbstractSessionCache#doGet(java.lang.String) - */ @Override public Session doGet(String id) { @@ -103,26 +99,32 @@ public class DefaultSessionCache extends AbstractSessionCache return session; } - /** - * @see org.eclipse.jetty.server.session.AbstractSessionCache#doPutIfAbsent(java.lang.String, org.eclipse.jetty.server.session.Session) - */ @Override public Session doPutIfAbsent(String id, Session session) { Session s = _sessions.putIfAbsent(id, session); - if (s == null && !(session instanceof PlaceHolderSession)) + if (s == null) _stats.increment(); return s; } - /** - * @see org.eclipse.jetty.server.session.AbstractSessionCache#doDelete(java.lang.String) - */ + @Override + protected Session doComputeIfAbsent(String id, Function mappingFunction) + { + return _sessions.computeIfAbsent(id, k -> + { + Session s = mappingFunction.apply(k); + if (s != null) + _stats.increment(); + return s; + }); + } + @Override public Session doDelete(String id) { Session s = _sessions.remove(id); - if (s != null && !(s instanceof PlaceHolderSession)) + if (s != null) _stats.decrement(); return s; } @@ -168,9 +170,6 @@ public class DefaultSessionCache extends AbstractSessionCache } } - /** - * @see org.eclipse.jetty.server.session.AbstractSessionCache#newSession(javax.servlet.http.HttpServletRequest, org.eclipse.jetty.server.session.SessionData) - */ @Override public Session newSession(HttpServletRequest request, SessionData data) { @@ -178,9 +177,6 @@ public class DefaultSessionCache extends AbstractSessionCache return s; } - /** - * @see org.eclipse.jetty.server.session.AbstractSessionCache#newSession(org.eclipse.jetty.server.session.SessionData) - */ @Override public Session newSession(SessionData data) { @@ -188,15 +184,10 @@ public class DefaultSessionCache extends AbstractSessionCache return s; } - /** - * @see org.eclipse.jetty.server.session.AbstractSessionCache#doReplace(java.lang.String, org.eclipse.jetty.server.session.Session, org.eclipse.jetty.server.session.Session) - */ @Override public boolean doReplace(String id, Session oldValue, Session newValue) { boolean result = _sessions.replace(id, oldValue, newValue); - if (result && (oldValue instanceof PlaceHolderSession)) - _stats.increment(); return result; } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/NullSessionCache.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/NullSessionCache.java index c589aab4bdb..ad9d05d3343 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/NullSessionCache.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/NullSessionCache.java @@ -18,6 +18,8 @@ package org.eclipse.jetty.server.session; +import java.util.function.Function; + import javax.servlet.http.HttpServletRequest; /** @@ -39,35 +41,23 @@ public class NullSessionCache extends AbstractSessionCache super.setEvictionPolicy(EVICT_ON_SESSION_EXIT); } - /** - * @see org.eclipse.jetty.server.session.SessionCache#shutdown() - */ @Override public void shutdown() { } - /** - * @see org.eclipse.jetty.server.session.AbstractSessionCache#newSession(org.eclipse.jetty.server.session.SessionData) - */ @Override public Session newSession(SessionData data) { return new Session(getSessionHandler(), data); } - /** - * @see org.eclipse.jetty.server.session.AbstractSessionCache#newSession(javax.servlet.http.HttpServletRequest, org.eclipse.jetty.server.session.SessionData) - */ @Override public Session newSession(HttpServletRequest request, SessionData data) { return new Session(getSessionHandler(), request, data); } - /** - * @see org.eclipse.jetty.server.session.AbstractSessionCache#doGet(java.lang.String) - */ @Override public Session doGet(String id) { @@ -75,9 +65,6 @@ public class NullSessionCache extends AbstractSessionCache return null; } - /** - * @see org.eclipse.jetty.server.session.AbstractSessionCache#doPutIfAbsent(java.lang.String, org.eclipse.jetty.server.session.Session) - */ @Override public Session doPutIfAbsent(String id, Session session) { @@ -85,9 +72,6 @@ public class NullSessionCache extends AbstractSessionCache return null; } - /** - * @see org.eclipse.jetty.server.session.AbstractSessionCache#doReplace(java.lang.String, org.eclipse.jetty.server.session.Session, org.eclipse.jetty.server.session.Session) - */ @Override public boolean doReplace(String id, Session oldValue, Session newValue) { @@ -95,21 +79,21 @@ public class NullSessionCache extends AbstractSessionCache return true; } - /** - * @see org.eclipse.jetty.server.session.AbstractSessionCache#doDelete(java.lang.String) - */ @Override public Session doDelete(String id) { return null; } - /** - * @see org.eclipse.jetty.server.session.AbstractSessionCache#setEvictionPolicy(int) - */ @Override public void setEvictionPolicy(int evictionTimeout) { LOG.warn("Ignoring eviction setting:" + evictionTimeout); } + + @Override + protected Session doComputeIfAbsent(String id, Function mappingFunction) + { + return mappingFunction.apply(id); + } } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/session/SessionCookieTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/session/SessionCookieTest.java index 36dd95d13a5..b5546f59e49 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/session/SessionCookieTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/session/SessionCookieTest.java @@ -19,6 +19,8 @@ package org.eclipse.jetty.server.session; import java.util.concurrent.TimeUnit; +import java.util.function.Function; + import javax.servlet.SessionCookieConfig; import javax.servlet.http.HttpServletRequest; @@ -35,10 +37,10 @@ import static org.junit.jupiter.api.Assertions.assertTrue; public class SessionCookieTest { - public class MockSessionStore extends AbstractSessionCache + public class MockSessionCache extends AbstractSessionCache { - public MockSessionStore(SessionHandler manager) + public MockSessionCache(SessionHandler manager) { super(manager); } @@ -83,6 +85,12 @@ public class SessionCookieTest { return null; } + + @Override + protected Session doComputeIfAbsent(String id, Function mappingFunction) + { + return mappingFunction.apply(id); + } } public class MockSessionIdManager extends DefaultSessionIdManager @@ -118,9 +126,9 @@ public class SessionCookieTest MockSessionIdManager idMgr = new MockSessionIdManager(server); idMgr.setWorkerName("node1"); SessionHandler mgr = new SessionHandler(); - MockSessionStore store = new MockSessionStore(mgr); - store.setSessionDataStore(new NullSessionDataStore()); - mgr.setSessionCache(store); + MockSessionCache cache = new MockSessionCache(mgr); + cache.setSessionDataStore(new NullSessionDataStore()); + mgr.setSessionCache(cache); mgr.setSessionIdManager(idMgr); long now = TimeUnit.NANOSECONDS.toMillis(System.nanoTime()); diff --git a/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractClusteredSessionScavengingTest.java b/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractClusteredSessionScavengingTest.java index b65cffe0fdc..253872010c1 100644 --- a/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractClusteredSessionScavengingTest.java +++ b/tests/test-sessions/test-sessions-common/src/main/java/org/eclipse/jetty/server/session/AbstractClusteredSessionScavengingTest.java @@ -69,6 +69,7 @@ public abstract class AbstractClusteredSessionScavengingTest extends AbstractTes DefaultSessionCacheFactory cacheFactory1 = new DefaultSessionCacheFactory(); cacheFactory1.setEvictionPolicy(SessionCache.NEVER_EVICT); //don't evict sessions + cacheFactory1.setFlushOnResponseCommit(true); SessionDataStoreFactory storeFactory1 = createSessionDataStoreFactory(); ((AbstractSessionDataStoreFactory)storeFactory1).setGracePeriodSec(scavengePeriod); ((AbstractSessionDataStoreFactory)storeFactory1).setSavePeriodSec(0); //always save when the session exits @@ -77,8 +78,6 @@ public abstract class AbstractClusteredSessionScavengingTest extends AbstractTes TestServlet servlet1 = new TestServlet(); ServletHolder holder1 = new ServletHolder(servlet1); ServletContextHandler context = server1.addContext(contextPath); - TestHttpChannelCompleteListener scopeListener = new TestHttpChannelCompleteListener(); - server1.getServerConnector().addBean(scopeListener); TestSessionListener listener1 = new TestSessionListener(); context.getSessionHandler().addEventListener(listener1); context.addServlet(holder1, servletMapping); @@ -91,13 +90,12 @@ public abstract class AbstractClusteredSessionScavengingTest extends AbstractTes DefaultSessionCacheFactory cacheFactory2 = new DefaultSessionCacheFactory(); cacheFactory2.setEvictionPolicy(SessionCache.NEVER_EVICT); //don't evict sessions + cacheFactory2.setFlushOnResponseCommit(true); SessionDataStoreFactory storeFactory2 = createSessionDataStoreFactory(); ((AbstractSessionDataStoreFactory)storeFactory2).setGracePeriodSec(scavengePeriod); ((AbstractSessionDataStoreFactory)storeFactory2).setSavePeriodSec(0); //always save when the session exits TestServer server2 = new TestServer(0, maxInactivePeriod, scavengePeriod, cacheFactory2, storeFactory2); ServletContextHandler context2 = server2.addContext(contextPath); - TestHttpChannelCompleteListener scopeListener2 = new TestHttpChannelCompleteListener(); - server2.getServerConnector().addBean(scopeListener2); context2.addServlet(TestServlet.class, servletMapping); SessionHandler m2 = context2.getSessionHandler(); @@ -110,18 +108,12 @@ public abstract class AbstractClusteredSessionScavengingTest extends AbstractTes try { // Perform one request to server1 to create a session - CountDownLatch synchronizer = new CountDownLatch(1); - scopeListener.setExitSynchronizer(synchronizer); ContentResponse response1 = client.GET("http://localhost:" + port1 + contextPath + servletMapping.substring(1) + "?action=init"); assertEquals(HttpServletResponse.SC_OK, response1.getStatus()); assertTrue(response1.getContentAsString().startsWith("init")); String sessionCookie = response1.getHeaders().get("Set-Cookie"); assertTrue(sessionCookie != null); String id = TestServer.extractSessionId(sessionCookie); - - //ensure request has finished being handled - synchronizer.await(5, TimeUnit.SECONDS); - assertEquals(1, ((DefaultSessionCache)m1.getSessionCache()).getSessionsCurrent()); assertEquals(1, ((DefaultSessionCache)m1.getSessionCache()).getSessionsMax()); assertEquals(1, ((DefaultSessionCache)m1.getSessionCache()).getSessionsTotal()); @@ -140,14 +132,10 @@ public abstract class AbstractClusteredSessionScavengingTest extends AbstractTes long time = start; while (time < end) { - synchronizer = new CountDownLatch(1); - scopeListener2.setExitSynchronizer(synchronizer); Request request = client.newRequest("http://localhost:" + port2 + contextPath + servletMapping.substring(1)); ContentResponse response2 = request.send(); assertEquals(HttpServletResponse.SC_OK, response2.getStatus()); assertTrue(response2.getContentAsString().startsWith("test")); - //ensure request has finished being handled - synchronizer.await(5, TimeUnit.SECONDS); Thread.sleep(requestInterval); assertSessionCounts(1, 1, 1, m2); time = System.currentTimeMillis();