From eeafb36c3f23fb1ed87d23b915969400f30e6d19 Mon Sep 17 00:00:00 2001 From: "Christopher L. Shannon" Date: Tue, 19 May 2020 07:51:17 -0400 Subject: [PATCH] Issue #4888 - Request getSession() now returns existing cached session if found Instead of logging and exception and returning null, if an existing Session is found in the session cache when attempting to add a new session then the existing session is used instead and returned Signed-off-by: Christopher L. Shannon --- .../org/eclipse/jetty/server/Request.java | 2 +- .../server/session/AbstractSessionCache.java | 28 +++- .../jetty/server/session/SessionCache.java | 12 ++ .../jetty/server/session/SessionHandler.java | 47 ++++++- .../session/DuplicateSessionCacheTest.java | 131 ++++++++++++++++++ 5 files changed, 210 insertions(+), 10 deletions(-) create mode 100644 tests/test-sessions/test-sessions-common/src/test/java/org/eclipse/jetty/server/session/DuplicateSessionCacheTest.java 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)); + } +}