diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 6565a6e7a7bf..7f969c4a9fea 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -1604,7 +1604,7 @@ public HttpSession getSession(boolean create) if (_sessionHandler == null) throw new IllegalStateException("No SessionManager"); - _session = _sessionHandler.newHttpSession(this); + _session = _sessionHandler.newHttpSession(this, true); HttpCookie cookie = _sessionHandler.getSessionCookie(_session, getContextPath(), isSecure()); if (cookie != null) _channel.getResponse().replaceCookie(cookie); 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 2029098e3f51..e0fc7bb81436 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 @@ -440,10 +440,31 @@ private Session loadSession(String id) */ @Override public void add(String id, Session session) throws Exception + { + if (addIfAbsent(id, session) != null) + { + throw new IllegalStateException("Session " + id + " already in cache"); + } + } + + /** + * Add an entirely new session (created by the application calling Request.getSession(true)) + * to the cache if absent. The usage count of the fresh session is incremented if added. + * If session already exists the existing session will be returned else null if it was added + * + * @param id the id + * @param session + * @return the existing Session object if one already exists for the given id, or null if no existing + * Session exists and the new one was added + */ + @Override + public Session addIfAbsent(String id, Session session) throws Exception { if (id == null || session == null) throw new IllegalArgumentException("Add key=" + id + " session=" + (session == null ? "null" : session.getId())); + Session existingSession; + try (Lock lock = session.lock()) { if (session.getSessionHandler() == null) @@ -452,14 +473,15 @@ public void add(String id, Session session) throws Exception if (!session.isValid()) throw new IllegalStateException("Session " + id + " is not valid"); - if (doPutIfAbsent(id, session) == null) + //Check if there is already an existing session for this id + if ((existingSession = doPutIfAbsent(id, session)) == null) { session.setResident(true); //its in the cache session.use(); //the request is using it } - else - throw new IllegalStateException("Session " + id + " already in cache"); } + + return existingSession; } /** diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionCache.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionCache.java index 734c6a15097b..058571808e9c 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionCache.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionCache.java @@ -129,6 +129,18 @@ default Session renewSessionId(String oldId, String newId, String oldExtendedId, */ void add(String id, Session session) throws Exception; + /** + * Adds a new Session to the cache if absent. + * If a session already exists in the cache for the given id then the existing Session is returned + * and the new Session is not added + * + * @param id + * @param session + * @throws Exception + * @return the existing Session object or null if one already exists for the given id + */ + Session addIfAbsent(String id, Session session) throws Exception; + /** * Get an existing Session. If necessary, the cache will load the data for * the session from the configured SessionDataStore. diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java b/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java index 0d0f72c6e204..67f0fc95b3fe 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/session/SessionHandler.java @@ -757,6 +757,20 @@ public String getExtendedId(HttpSession session) * @return the new HttpSession */ public HttpSession newHttpSession(HttpServletRequest request) + { + return newHttpSession(request, false); + } + + /** + * Creates a new HttpSession or returns an existing HttpSession + * if useExisting is true + * + * @param request the HttpServletRequest containing the requested session id + * @param useExisting true to re-use an existing cached session if exists; + * false to throw an exception on existing cached session + * @return the new HttpSession or existing HttpSession if exists + */ + public HttpSession newHttpSession(HttpServletRequest request, boolean useExisting) { long created = System.currentTimeMillis(); String id = _sessionIdManager.newSessionId(request, created); @@ -766,15 +780,36 @@ public HttpSession newHttpSession(HttpServletRequest request) try { - _sessionCache.add(id, session); - Request.getBaseRequest(request).enterSession(session); - _sessionsCreatedStats.increment(); + final Session existingSession; + //If it is ok to use an existing cached session then use addIfAbsent + if (useExisting) + { + existingSession = _sessionCache.addIfAbsent(id, session); + } + else + { + existingSession = null; + //This will throw an exception if an existing Session is already in the cache + _sessionCache.add(id, session); + } - if (request != null && request.isSecure()) - session.setAttribute(Session.SESSION_CREATED_SECURE, Boolean.TRUE); + //Check if the returned session is null + //If it is then that means the new session was added + if (existingSession == null) + { + Request.getBaseRequest(request).enterSession(session); + _sessionsCreatedStats.increment(); - callSessionCreatedListeners(session); + if (request != null && request.isSecure()) + session.setAttribute(Session.SESSION_CREATED_SECURE, Boolean.TRUE); + callSessionCreatedListeners(session); + } + else + { + //We found an existing session for this id so return that instead + session = existingSession; + } return session; } catch (Exception e) diff --git a/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/DuplicateSessionCacheTest.java b/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/DuplicateSessionCacheTest.java new file mode 100644 index 000000000000..9c220bfa97e6 --- /dev/null +++ b/tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/DuplicateSessionCacheTest.java @@ -0,0 +1,131 @@ +// +// ======================================================================== +// Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.server.session; + +import javax.servlet.http.HttpSession; + +import org.eclipse.jetty.client.HttpClient; +import org.eclipse.jetty.server.Request; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.server.handler.DefaultHandler; +import org.eclipse.jetty.server.handler.HandlerList; +import org.eclipse.jetty.servlet.DefaultServlet; +import org.eclipse.jetty.servlet.ServletContextHandler; +import org.eclipse.jetty.util.component.LifeCycle; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class DuplicateSessionCacheTest +{ + private Server server; + private HttpClient client; + + ServletContextHandler contextHandler; + + @BeforeEach + public void startServer() throws Exception + { + server = new Server(); + ServerConnector connector = new ServerConnector(server); + connector.setPort(0); + server.addConnector(connector); + + // Default session behavior + contextHandler = new ServletContextHandler(ServletContextHandler.SESSIONS); + contextHandler.setContextPath("/"); + contextHandler.addServlet(DefaultServlet.class, "/"); + + HandlerList handlers = new HandlerList(); + handlers.addHandler(contextHandler); + handlers.addHandler(new DefaultHandler()); + + server.setHandler(handlers); + server.start(); + } + + @AfterEach + public void stopServerAndClient() + { + LifeCycle.stop(server); + LifeCycle.stop(client); + } + + @BeforeEach + public void startClient() throws Exception + { + client = new HttpClient(); + client.start(); + } + + @Test + public void testNewSessionUseExistingTrue() throws Exception + { + + //Create a new session and get the Id + Request request1 = new Request(null, null); + HttpSession session1 = contextHandler.getSessionHandler().newHttpSession(request1); + String id = session1.getId(); + + //Re-use the same session id and try and create a new session + //With version 9.4.20 this would work but return a new session which is not ideal if one exists + //After version 9.4.21 this would throw an exception + //With the commit this test is included in now the existing session will be returned + Request request2 = new Request(null, null); + request2.setRequestedSessionId(id); + HttpSession session2 = contextHandler.getSessionHandler().newHttpSession(request2,true); + + //Verify the two returned sessions are the same + assertTrue(session1 == session2); + assertEquals(id, session1.getId()); + } + + @Test + public void testUseExistingCachedFalse() throws Exception + { + //Create a new session and get the Id + Request request1 = new Request(null, null); + HttpSession session1 = contextHandler.getSessionHandler().newHttpSession(request1); + String id = session1.getId(); + + //Should return null because a session exists and useExisting is false + Request request2 = new Request(null, null); + request2.setRequestedSessionId(id); + assertNull(contextHandler.getSessionHandler().newHttpSession(request2, false)); + } + + @Test + public void testNewHttpSessionOriginalMethod() throws Exception + { + //Create a new session and get the Id + Request request1 = new Request(null, null); + HttpSession session1 = contextHandler.getSessionHandler().newHttpSession(request1); + String id = session1.getId(); + + //Should return null because a session exists + Request request2 = new Request(null, null); + request2.setRequestedSessionId(id); + assertNull(contextHandler.getSessionHandler().newHttpSession(request2)); + } +}