400144 When loading a session fails the JDBCSessionManger produces duplicate session IDs

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

@@ -821,7 +821,7 @@ public class JDBCSessionIdManager extends AbstractSessionIdManager
                     statement = connection.prepareStatement(_deleteOldExpiredSessions);
                     statement.setLong(1, upperBound);
                     int rows = statement.executeUpdate();
-                    if (LOG.isDebugEnabled()) LOG.debug("Deleted "+rows+" rows");
+                    if (LOG.isDebugEnabled()) LOG.debug("Deleted "+rows+" rows of old sessions expired before "+upperBound);
                 }
             }
         }

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

@@ -279,6 +279,8 @@ public class JDBCSessionManager extends AbstractSessionManager
                 return false;
             }
         }
+        
+
 
         /**
          * Exit from session
@@ -330,7 +332,7 @@ public class JDBCSessionManager extends AbstractSessionManager
             return "Session rowId="+_rowId+",id="+getId()+",lastNode="+_lastNode+
                             ",created="+getCreationTime()+",accessed="+getAccessed()+
                             ",lastAccessed="+getLastAccessedTime()+",cookieSet="+_cookieSet+
-                            "lastSaved="+_lastSaved;
+                            ",lastSaved="+_lastSaved+",expiry="+_expiryTime;
         }
     }
 
@@ -440,8 +442,6 @@ public class JDBCSessionManager extends AbstractSessionManager
 
         synchronized (this)
         {
-            try
-            {
                 //check if we need to reload the session -
                 //as an optimization, don't reload on every access
                 //to reduce the load on the database. This introduces a window of
@@ -469,22 +469,31 @@ public class JDBCSessionManager extends AbstractSessionManager
                                 " difference="+(now - memSession._lastSaved));
                 }
 
-                if (memSession==null)
+                try
                 {
-                    LOG.debug("getSession("+idInCluster+"): no session in session map. Reloading session data from db.");
+                    if (memSession==null)
-                    session = loadSession(idInCluster, canonicalize(_context.getContextPath()), getVirtualHost(_context));
+                    {
+                        LOG.debug("getSession("+idInCluster+"): no session in session map. Reloading session data from db.");
+                        session = loadSession(idInCluster, canonicalize(_context.getContextPath()), getVirtualHost(_context));
+                    }
+                    else if ((now - memSession._lastSaved) >= (_saveIntervalSec * 1000L))
+                    {
+                        LOG.debug("getSession("+idInCluster+"): stale session. Reloading session data from db.");
+                        session = loadSession(idInCluster, canonicalize(_context.getContextPath()), getVirtualHost(_context));
+                    }
+                    else
+                    {
+                        LOG.debug("getSession("+idInCluster+"): session in session map");
+                        session = memSession;
+                    }
                 }
-                else if ((now - memSession._lastSaved) >= (_saveIntervalSec * 1000L))
+                catch (Exception e)
                 {
-                    LOG.debug("getSession("+idInCluster+"): stale session. Reloading session data from db.");
+                    LOG.warn("Unable to load session "+idInCluster, e);
-                    session = loadSession(idInCluster, canonicalize(_context.getContextPath()), getVirtualHost(_context));
+                    return null;
-                }
-                else
-                {
-                    LOG.debug("getSession("+idInCluster+"): session in session map");
-                    session = memSession;
                 }
 
+                
                 //If we have a session
                 if (session != null)
                 {
@@ -494,13 +503,23 @@ public class JDBCSessionManager extends AbstractSessionManager
                         //if session doesn't expire, or has not already expired, update it and put it in this nodes' memory
                         if (session._expiryTime <= 0 || session._expiryTime > now)
                         {
-                            if (LOG.isDebugEnabled()) LOG.debug("getSession("+idInCluster+"): lastNode="+session.getLastNode()+" thisNode="+getSessionIdManager().getWorkerName());
+                            if (LOG.isDebugEnabled()) 
+                                LOG.debug("getSession("+idInCluster+"): lastNode="+session.getLastNode()+" thisNode="+getSessionIdManager().getWorkerName());
+                            
                             session.setLastNode(getSessionIdManager().getWorkerName());                            
                             _sessions.put(idInCluster, session);
-                            session.didActivate();
+                            
-                            //TODO is this the best way to do this? Or do this on the way out using
+                            //update in db: if unable to update, session will be scavenged later
-                            //the _dirty flag?
+                            try
-                            updateSessionNode(session);
+                            {
+                                updateSessionNode(session);
+                                session.didActivate();
+                            }
+                            catch (Exception e)
+                            {
+                                LOG.warn("Unable to update freshly loaded session "+idInCluster, e);
+                                return null;
+                            }
                         }
                         else
                         {
@@ -519,12 +538,6 @@ public class JDBCSessionManager extends AbstractSessionManager
                 }
 
                 return session;
-            }
-            catch (Exception e)
-            {
-                LOG.warn("Unable to get session", e);
-                return null;
-            }
         }
     }
 
@@ -840,7 +853,12 @@ public class JDBCSessionManager extends AbstractSessionManager
             _context.getContextHandler().handle(load);
 
         if (_exception.get()!=null)
+        {
+            //if the session could not be restored, take its id out of the pool of currently-in-use
+            //session ids
+            _jdbcSessionIdMgr.removeSession(id);
             throw _exception.get();
+        }
 
         return _reference.get();
     }

tests/test-sessions/test-jdbc-sessions/src/test/java/org/eclipse/jetty/server/session/JdbcTestServer.java

@@ -53,11 +53,7 @@ public class JdbcTestServer extends AbstractTestServer
         super(port, maxInactivePeriod, scavengePeriod, DEFAULT_CONNECTION_URL);
     }
     
-    public JdbcTestServer (int port, boolean optimize)
+ 
-    {
-        super(port);
-    }
-
     /** 
      * @see org.eclipse.jetty.server.session.AbstractTestServer#newSessionHandler(org.eclipse.jetty.server.SessionManager)
      */

tests/test-sessions/test-jdbc-sessions/src/test/java/org/eclipse/jetty/server/session/ReloadedSessionMissingClassTest.java

@@ -0,0 +1,153 @@
+//
+//  ========================================================================
+//  Copyright (c) 1995-2013 Mort Bay Consulting Pty. Ltd.
+//  ------------------------------------------------------------------------
+//  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 static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.io.FileWriter;
+import java.net.URL;
+import java.net.URLClassLoader;
+
+import javax.servlet.http.HttpServletResponse;
+
+import org.eclipse.jetty.client.ContentExchange;
+import org.eclipse.jetty.client.HttpClient;
+import org.eclipse.jetty.http.HttpMethods;
+import org.eclipse.jetty.util.IO;
+import org.eclipse.jetty.util.log.Log;
+import org.eclipse.jetty.util.log.StdErrLog;
+import org.eclipse.jetty.webapp.WebAppContext;
+import org.junit.Test;
+
+/**
+ * ReloadedSessionMissingClassTest
+ *
+ *
+ *
+ */
+public class ReloadedSessionMissingClassTest
+{
+    
+    @Test
+    public void testSessionReloadWithMissingClass() throws Exception
+    {
+        ((StdErrLog)Log.getLogger(org.eclipse.jetty.server.session.JDBCSessionManager.class)).setHideStacks(true);
+        String contextPath = "/foo";
+        File srcDir = new File(System.getProperty("basedir"), "src");
+        File targetDir = new File(System.getProperty("basedir"), "target");
+        File testDir = new File (srcDir, "test");
+        File resourcesDir = new File (testDir, "resources");
+
+        File unpackedWarDir = new File (targetDir, "foo");
+        if (unpackedWarDir.exists())
+            IO.delete(unpackedWarDir);
+        unpackedWarDir.mkdir();
+
+        File webInfDir = new File (unpackedWarDir, "WEB-INF");
+        webInfDir.mkdir();
+
+        File webXml = new File(webInfDir, "web.xml");
+        String xml =
+            "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n" +
+            "<web-app xmlns=\"http://java.sun.com/xml/ns/j2ee\"\n" +
+            "         xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\"\n" +
+            "         xsi:schemaLocation=\"http://java.sun.com/xml/ns/j2ee http://java.sun.com/xml/ns/j2ee/web-app_2_4.xsd\"\n" +
+            "         version=\"2.4\">\n" +
+            "\n" +
+            "<session-config>\n"+
+            " <session-timeout>1</session-timeout>\n" +
+            "</session-config>\n"+
+            "</web-app>";
+        FileWriter w = new FileWriter(webXml);
+        w.write(xml);
+        w.close();
+
+        File foobarJar = new File (resourcesDir, "foobar.jar");
+        File foobarNOfooJar = new File (resourcesDir, "foobarNOfoo.jar");
+        
+        URL[] foobarUrls = new URL[]{foobarJar.toURI().toURL()};
+        URL[] barUrls = new URL[]{foobarNOfooJar.toURI().toURL()};
+        
+        URLClassLoader loaderWithFoo = new URLClassLoader(foobarUrls, Thread.currentThread().getContextClassLoader());
+        URLClassLoader loaderWithoutFoo = new URLClassLoader(barUrls, Thread.currentThread().getContextClassLoader());
+
+       
+        AbstractTestServer server1 = new JdbcTestServer(0);
+        WebAppContext webApp = server1.addWebAppContext(unpackedWarDir.getCanonicalPath(), contextPath);
+        webApp.setClassLoader(loaderWithFoo);
+        webApp.addServlet("Bar", "/bar");
+        server1.start();
+        int port1 = server1.getPort();
+        try
+        {
+            HttpClient client = new HttpClient();
+            client.setConnectorType(HttpClient.CONNECTOR_SOCKET);
+            client.start();
+            try
+            {
+                // Perform one request to server1 to create a session
+                ContentExchange exchange1 = new ContentExchange(true);
+                exchange1.setMethod(HttpMethods.GET);
+                exchange1.setURL("http://localhost:" + port1 + contextPath +"/bar?action=set");
+                client.send(exchange1);
+                exchange1.waitForDone();
+                assertEquals( HttpServletResponse.SC_OK, exchange1.getResponseStatus());
+                String sessionCookie = exchange1.getResponseFields().getStringField("Set-Cookie");
+                assertTrue(sessionCookie != null);
+                String sessionId = (String)webApp.getServletContext().getAttribute("foo");
+                assertNotNull(sessionId);
+                // Mangle the cookie, replacing Path with $Path, etc.
+                sessionCookie = sessionCookie.replaceFirst("(\\W)(P|p)ath=", "$1\\$Path=");
+                
+                //Stop the webapp
+                webApp.stop();
+                
+                webApp.setClassLoader(loaderWithoutFoo);
+                webApp.addServlet("Bar", "/bar");
+                
+                //restart webapp
+                webApp.start();
+                
+                ContentExchange exchange2 = new ContentExchange(true);
+                exchange2.setMethod(HttpMethods.GET);
+                exchange2.setURL("http://localhost:" + port1 + contextPath + "/bar?action=get");
+                exchange2.getRequestFields().add("Cookie", sessionCookie);
+                client.send(exchange2);
+                exchange2.waitForDone();
+                assertEquals(HttpServletResponse.SC_OK,exchange2.getResponseStatus());
+                String afterStopSessionId = (String)webApp.getServletContext().getAttribute("foo.session");
+                
+                assertNotNull(afterStopSessionId);
+                assertTrue(!afterStopSessionId.equals(sessionId));  
+
+            }
+            finally
+            {
+                client.stop();
+            }
+        }
+        finally
+        {
+            server1.stop();
+        }
+    }
+}