Fix bug 43343. Correctly handle the case where a request arrives for a session we...

git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@652662 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/java/org/apache/catalina/session/PersistentManagerBase.java b/java/org/apache/catalina/session/PersistentManagerBase.java
index 9fc4486..ce8fc11 100644 (file)
--- a/java/org/apache/catalina/session/PersistentManagerBase.java
+++ b/java/org/apache/catalina/session/PersistentManagerBase.java
@@ -590,6 +590,23 @@ public abstract class PersistentManagerBase
     public Session findSession(String id) throws IOException {
 
         Session session = super.findSession(id);
+        // OK, at this point, we're not sure if another thread is trying to
+        // remove the session or not so the only way around this is to lock it
+        // (or attempt to) and then try to get it by this session id again. If
+        // the other code ran swapOut, then we should get a null back during
+        // this run, and if not, we lock it out so we can access the session
+        // safely.
+        if(session != null) {
+            synchronized(session){
+                session = super.findSession(session.getIdInternal());
+                if(session != null){
+                   // To keep any external calling code from messing up the
+                   // concurrency.
+                   session.access();
+                   session.endAccess();
+                }
+            }
+        }
         if (session != null)
             return (session);
 
@@ -1024,24 +1041,24 @@ public abstract class PersistentManagerBase
         long timeNow = System.currentTimeMillis();
 
         // Swap out all sessions idle longer than maxIdleSwap
-        // FIXME: What's preventing us from mangling a session during
-        // a request?
         if (maxIdleSwap >= 0) {
             for (int i = 0; i < sessions.length; i++) {
                 StandardSession session = (StandardSession) sessions[i];
-                if (!session.isValid())
-                    continue;
-                int timeIdle = // Truncate, do not round up
-                    (int) ((timeNow - session.getLastAccessedTime()) / 1000L);
-                if (timeIdle > maxIdleSwap && timeIdle > minIdleSwap) {
-                    if (log.isDebugEnabled())
-                        log.debug(sm.getString
-                            ("persistentManager.swapMaxIdle",
-                             session.getIdInternal(), new Integer(timeIdle)));
-                    try {
-                        swapOut(session);
-                    } catch (IOException e) {
-                        ;   // This is logged in writeSession()
+                synchronized (session) {
+                    if (!session.isValid())
+                        continue;
+                    int timeIdle = // Truncate, do not round up
+                        (int) ((timeNow - session.getLastAccessedTime()) / 1000L);
+                    if (timeIdle > maxIdleSwap && timeIdle > minIdleSwap) {
+                        if (log.isDebugEnabled())
+                            log.debug(sm.getString
+                                ("persistentManager.swapMaxIdle",
+                                 session.getIdInternal(), new Integer(timeIdle)));
+                        try {
+                            swapOut(session);
+                        } catch (IOException e) {
+                            ;   // This is logged in writeSession()
+                        }
                     }
                 }
             }
@@ -1073,19 +1090,21 @@ public abstract class PersistentManagerBase
         long timeNow = System.currentTimeMillis();
 
         for (int i = 0; i < sessions.length && toswap > 0; i++) {
-            int timeIdle = // Truncate, do not round up
-                (int) ((timeNow - sessions[i].getLastAccessedTime()) / 1000L);
-            if (timeIdle > minIdleSwap) {
-                if(log.isDebugEnabled())
-                    log.debug(sm.getString
-                        ("persistentManager.swapTooManyActive",
-                         sessions[i].getIdInternal(), new Integer(timeIdle)));
-                try {
-                    swapOut(sessions[i]);
-                } catch (IOException e) {
-                    ;   // This is logged in writeSession()
+            synchronized (sessions[i]) {
+                int timeIdle = // Truncate, do not round up
+                    (int) ((timeNow - sessions[i].getLastAccessedTime()) / 1000L);
+                if (timeIdle > minIdleSwap) {
+                    if(log.isDebugEnabled())
+                        log.debug(sm.getString
+                            ("persistentManager.swapTooManyActive",
+                             sessions[i].getIdInternal(), new Integer(timeIdle)));
+                    try {
+                        swapOut(sessions[i]);
+                    } catch (IOException e) {
+                        ;   // This is logged in writeSession()
+                    }
+                    toswap--;
                 }
-                toswap--;
             }
         }
 
@@ -1107,20 +1126,22 @@ public abstract class PersistentManagerBase
         if (maxIdleBackup >= 0) {
             for (int i = 0; i < sessions.length; i++) {
                 StandardSession session = (StandardSession) sessions[i];
-                if (!session.isValid())
-                    continue;
-                int timeIdle = // Truncate, do not round up
-                    (int) ((timeNow - session.getLastAccessedTime()) / 1000L);
-                if (timeIdle > maxIdleBackup) {
-                    if (log.isDebugEnabled())
-                        log.debug(sm.getString
-                            ("persistentManager.backupMaxIdle",
-                            session.getIdInternal(), new Integer(timeIdle)));
-
-                    try {
-                        writeSession(session);
-                    } catch (IOException e) {
-                        ;   // This is logged in writeSession()
+                synchronized (session) {
+                    if (!session.isValid())
+                        continue;
+                    int timeIdle = // Truncate, do not round up
+                        (int) ((timeNow - session.getLastAccessedTime()) / 1000L);
+                    if (timeIdle > maxIdleBackup) {
+                        if (log.isDebugEnabled())
+                            log.debug(sm.getString
+                                ("persistentManager.backupMaxIdle",
+                                session.getIdInternal(), new Integer(timeIdle)));
+    
+                        try {
+                            writeSession(session);
+                        } catch (IOException e) {
+                            ;   // This is logged in writeSession()
+                        }
                     }
                 }
             }

diff --git a/java/org/apache/catalina/valves/PersistentValve.java b/java/org/apache/catalina/valves/PersistentValve.java
index 1b7da1e..bb072a1 100644 (file)
--- a/java/org/apache/catalina/valves/PersistentValve.java
+++ b/java/org/apache/catalina/valves/PersistentValve.java
@@ -30,16 +30,18 @@ import org.apache.catalina.Session;
 import org.apache.catalina.Store;
 import org.apache.catalina.connector.Request;
 import org.apache.catalina.connector.Response;
-import org.apache.catalina.core.StandardHost;
 import org.apache.catalina.session.PersistentManager;
 import org.apache.catalina.util.StringManager;
 
 
 /**
- * Valve that implements the default basic behavior for the
- * <code>StandardHost</code> container implementation.
+ * Valve that implements per-request session persistence. It is intended to be
+ * used with non-sticky load-balancers.
  * <p>
  * <b>USAGE CONSTRAINT</b>: To work correctly it requires a  PersistentManager.
+ * <p>
+ * <b>USAGE CONSTRAINT</b>: To work correctly it assumes only one request exists
+ *                              per session at any one time.
  *
  * @author Jean-Frederic Clere
  * @version $Revision$ $Date$
@@ -97,7 +99,6 @@ public class PersistentValve
         throws IOException, ServletException {
 
         // Select the Context to be used for this Request
-        StandardHost host = (StandardHost) getContainer();
         Context context = request.getContext();
         if (context == null) {
             response.sendError