There are four Tomcat 4 bug reports filed on this problem.

There was one bug report filed for Tomcat 5 and Remy applied a
small patch to fix it.

Problems with expiring sessions have been reported numerous times.
The basic problem is as follows, starting at time 0 min and with
a max inactive interval of 30 minutes:

00 min: First request, new session created, LastAccessedTime 0
02 min: Second request, reuse session, LastAccessedTime 0
31 min: Third request, reuse session, LastAccessedTime now 2
33 min: Background manager thread expires session even though
        it has only been two minutes since the remote clients
        last request.

The argument for not changing how this works is based on how
the Servlet Spec defines Session.getLastAccessedTime().

But I agree with all those who have complained about this
behaviour that Tomcat session timeouts are buggy.

So I came up with a compromise that still allows the
HttpSession.getLastAccessedTime() to return the time of the
previous request to meet the servlet spec.
But internally sessions are expired when
current time > last request + max inactive interval.

When we do a major revision we should consider adding
the StandardSession.getLastUsedTime() method to the
org.apache.catalina.Session interface.

JDBCStore
---------

The JDBCStore required a great deal of unnecessary db
queries to manage the persisted data. This could severly
impact its ability to scale to large numbers of sessions.

1. When a JSESSIONID cookie was submitted with a request where
   the Session no longer exists multiple queries of the db occurred
   to try and load a persisted Session from the Store. I was
   seeing four attempts to load from the persistence store
   each request when a Session did not exist for a JSESSIONID.

   PersistentManagerBase swapIn() and swapOut() were patched
   to maintain a Hashtable of JSESSIONID's which do not exist
   in the Store so that they don't have to be checked multiple
   times.  Each checkInterval the Hashtable is cleared to
   prevent it from consuming too much memory.

2. The StoreBase.processExpires() method triggers a load of
   each Session persisted to the db each checkInterval to
   perform its test to determine if the Session has expired.
   This incurred alot of overhead on the db, especially
   if there was a large amount of session data. The number
   of queries performed each checkInterval is 1 + number of
   sessions persisted to the db + number of expired sessions
   removed.

   The StoreBase.processExpires() method was overridden
   in JDBCStore.  The method in JDBCStore performs a
   query of the db to find only those Sessions which should
   be expired. The number of queries performed here is 1 +
   2 * the number of expired sessions (load then remove
   of expired session).

3. JDBCStore.remove() is being called sometimes with a null
   sessionid String causing an unnecessary synchronization
   and db query.

   Added a check for a null sessionid String at top of method.

I propose that these patched be applied to Tomcat 4 first,
then to Tomcat 5.

Regards,

Glenn
On Fri, Jan 02, 2004 at 11:10:16AM +0200, [EMAIL PROTECTED] wrote:
> Reading the servlet spec raised a couple of thoughts about http session
> handling to my mind. I did verify them, but did not file bug reports.
> 
> Should I write a patch for these?
> 
> 
> Thought #1
> ==
> 
> "SRV.7.6 Last Accessed Times
> The getLastAccessedTime method of the HttpSession interface allows a servlet
> to determine the last time the session was accessed before the current
> request. The session is considered to be accessed when a request that is part
> of the session is first handled by the servlet container."
> 
> Imagine the following situation with four requests in the same session:
> 
> Moment 0: Request #0 arrives. The session is initiated.
> Moment 1: Request #1 arrives. The request processing performs some long
> operation.
> Moment 2: Request #2 arrives.
> Moment 3: Request #3 arrives.
> Moment 4: The long operation of the request #1 processing is complete. Request
> #1 processing calls session.getLastAccessedTime(). According to the spec the
> method should return the time of moment 0 (request #0 was the previous
> request before the request #1). Tomcat returns the time of moment 2 (the time
> request #2 arrived) instead.
> 
> 
> Thought #2
> ==
> 
> If the session is created by the current request, the
> session.getLastAccessedTime() returns the session creation time. Should it
> return 0 instead? I'd find it a bit less incorrect.
> 
> 
> Thought #3
> ==
> 
> "SRV.7.5 Session Timeouts
> The session invalidation will not take effect until all servlets using that
> session have exited the service method."
> 
> Tomcat does nothing to ensure this.
> 
> To reproduce, set session timeout to 3mins and put the following code to
> service method:
> 
> HttpSession session = request.getSession();
> Thread.sleep(200 * 1000L); // a long operation =)
> session.getLastAccessedTime();
> 
> ->IllegalStateException is thrown
> 
> --
> Jarno Peltoniemi
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [EMAIL PROTECTED]
> For additional commands, e-mail: [EMAIL PROTECTED]
----------------------------------------------------------------------
Glenn Nielsen             [EMAIL PROTECTED] | /* Spelin donut madder    |
MOREnet System Programming               |  * if iz ina coment.      |
Missouri Research and Education Network  |  */                       |
----------------------------------------------------------------------
Index: JDBCStore.java
===================================================================
RCS file: 
/home/cvs/jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/session/JDBCStore.java,v
retrieving revision 1.12
diff -u -r1.12 JDBCStore.java
--- JDBCStore.java      20 Mar 2004 10:57:18 -0000      1.12
+++ JDBCStore.java      20 Apr 2004 14:30:26 -0000
@@ -201,6 +201,11 @@
      */
     protected PreparedStatement preparedLoadSql = null;
 
+    /**
+     * Variable to hold the <code>processExpires()</code> prepared statement.
+     */
+    protected PreparedStatement preparedExpiresSql = null;
+
     // ------------------------------------------------------------- Properties
 
     /**
@@ -630,6 +635,11 @@
      * @exception IOException if an input/output error occurs
      */
     public void remove(String id) throws IOException {
+
+        if (id == null) {
+            return;
+        }
+
         String removeSql =
             "DELETE FROM " + sessionTable + " WHERE " + sessionIdCol +
             " = ?  AND " + sessionAppCol + " = ?";
@@ -742,7 +752,7 @@
                 preparedSaveSql.setBinaryStream(3, in, size);
                 preparedSaveSql.setString(4, session.isValid()?"1":"0");
                 preparedSaveSql.setInt(5, session.getMaxInactiveInterval());
-                preparedSaveSql.setLong(6, session.getLastAccessedTime());
+                preparedSaveSql.setLong(6, 
((StandardSession)session).getLastUsedTime());
                 preparedSaveSql.execute();
             } catch(SQLException e) {
                 log(sm.getString(getStoreName()+".SQLException", e));
@@ -769,6 +779,101 @@
     // --------------------------------------------------------- Protected Methods
 
     /**
+     * Called by our background reaper thread to check if Sessions
+     * saved in our store are subject of being expired. If so expire
+     * the Session and remove it from the Store.
+     *
+     */
+    protected void processExpires() {
+
+        if(!started) {
+            return;
+        }
+
+        String expiresSql =
+            "SELECT " + sessionIdCol + " FROM " + sessionTable +
+            " WHERE " + sessionAppCol + " = ? AND ? > (" +
+            sessionLastAccessedCol + " + (" + sessionMaxInactiveCol +
+            "*1000))";
+
+        ResultSet rst = null;
+        String keys[] = null;
+        long timeNow = System.currentTimeMillis();
+
+        synchronized(this) {
+            Connection _conn = getConnection();
+
+            if(_conn == null) {
+                return;
+            }
+
+            try {
+                if(preparedExpiresSql == null) {
+                    preparedExpiresSql = _conn.prepareStatement(expiresSql);
+                }
+
+                preparedExpiresSql.setString(1, getName());
+                preparedExpiresSql.setLong(2,timeNow);
+                rst = preparedExpiresSql.executeQuery();
+                ArrayList tmpkeys = new ArrayList();
+                if (rst != null) {
+                    while(rst.next()) {
+                        tmpkeys.add(rst.getString(1));
+                    }
+                }
+                keys = (String[]) tmpkeys.toArray(new String[tmpkeys.size()]);
+            } catch(SQLException e) {
+                log(sm.getString(getStoreName()+".SQLException", e));
+                keys = new String[0];
+            } finally {
+                try {
+                    if(rst != null) {
+                        rst.close();
+                    }
+                } catch(SQLException e) {
+                    ;
+                }
+
+                release(_conn);
+            }
+        }
+
+        for (int i = 0; i < keys.length; i++) {
+            try {
+                StandardSession session = (StandardSession) load(keys[i]);
+                if (session == null) {
+                    continue;
+                }
+                if (!session.isValid()) {
+                    continue;
+                }
+                int maxInactiveInterval = session.getMaxInactiveInterval();
+                if (maxInactiveInterval < 0) {
+                    continue;
+                }
+                int timeIdle = // Truncate, do not round up
+                    (int) ((timeNow - ((StandardSession)session).getLastUsedTime()) / 
1000L);
+                if (timeIdle >= maxInactiveInterval) {
+                    if ( ( (PersistentManagerBase) manager).isLoaded( keys[i] )) {
+                        // recycle old backup session
+                        session.recycle();
+                    } else {
+                        // expire swapped out session
+                        session.expire();
+                    }
+                    remove(session.getId());
+                }
+            } catch (IOException e) {
+                log (e.toString());
+                e.printStackTrace();
+            } catch (ClassNotFoundException e) {
+                log (e.toString());
+                e.printStackTrace();
+            }
+        }
+    }
+
+    /**
      * Check the connection associated with this store, if it's
      * <code>null</code> or closed try to reopen it.
      * Returns <code>null</code> if the connection could not be established.
@@ -882,6 +987,14 @@
                 }
             }
 
+            if( preparedExpiresSql != null ) {
+                try {
+                    preparedExpiresSql.close();
+                } catch (SQLException e) {
+                    ;
+                }
+            }
+
             try {
                 conn.close();
             } catch (SQLException e) {
@@ -894,6 +1007,7 @@
             this.preparedClearSql = null;
             this.preparedRemoveSql = null;
             this.preparedLoadSql = null;
+            this.preparedExpiresSql = null;
             this.conn = null;
         }
     }
Index: PersistentManagerBase.java
===================================================================
RCS file: 
/home/cvs/jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/session/PersistentManagerBase.java,v
retrieving revision 1.17
diff -u -r1.17 PersistentManagerBase.java
--- PersistentManagerBase.java  11 Dec 2003 23:52:04 -0000      1.17
+++ PersistentManagerBase.java  20 Apr 2004 14:30:26 -0000
@@ -67,6 +67,7 @@
 import java.beans.PropertyChangeEvent;
 import java.beans.PropertyChangeListener;
 import java.io.IOException;
+import java.util.Hashtable;
 import org.apache.catalina.Container;
 import org.apache.catalina.Context;
 import org.apache.catalina.Lifecycle;
@@ -135,6 +136,20 @@
 
 
     /**
+     * Map of Sessions which are not in swap but have
+     * been requested at least once.
+     *
+     * This is to improve performance when a JSESSIONID
+     * is sent by the client but no longer exists as a
+     * session so that the Store doesn't get banged on
+     * multiple times per request.
+     *
+     * This is a Hashtable to ensure use is thread safe.
+     *
+     */
+    private Hashtable sessionSwapIgnore = new Hashtable();
+
+    /**
      * The background thread.
      */
     private Thread thread = null;
@@ -691,6 +706,10 @@
         if (store == null)
             return null;
 
+        if (sessionSwapIgnore.contains(id)) {
+            return null;
+        }
+
         Session session = null;
         try {
             session = store.load(id);
@@ -700,14 +719,17 @@
                 (sm.getString("persistentManager.deserializeError", id, e));
         }
 
-        if (session == null)
+        if (session == null) {
+            sessionSwapIgnore.put(id,id);
             return (null);
+        }
 
         if (!session.isValid()
                 || isSessionStale(session, System.currentTimeMillis())) {
             log("session swapped in is invalid or expired");
             session.expire();
             store.remove(id);
+            sessionSwapIgnore.put(id,id);
             return (null);
         }
 
@@ -762,6 +784,7 @@
 
         try {
             store.save(session);
+            sessionSwapIgnore.remove(session.getId());
         } catch (IOException e) {
             log(sm.getString
                 ("persistentManager.serializeError", session.getId(), e));
@@ -938,8 +961,9 @@
 
         int maxInactiveInterval = session.getMaxInactiveInterval();
         if (maxInactiveInterval >= 0) {
+            long lastAccessed = ((StandardSession)session).getLastUsedTime();
             int timeIdle = // Truncate, do not round up
-                (int) ((timeNow - session.getLastAccessedTime()) / 1000L);
+                (int) ((timeNow - lastAccessed) / 1000L);
             if (timeIdle >= maxInactiveInterval)
                 return true;
         }
@@ -994,8 +1018,9 @@
                 StandardSession session = (StandardSession) sessions[i];
                 if (!session.isValid())
                     continue;
+                long lastAccessed = ((StandardSession)session).getLastUsedTime();
                 int timeIdle = // Truncate, do not round up
-                    (int) ((timeNow - session.getLastAccessedTime()) / 1000L);
+                    (int) ((timeNow - lastAccessed) / 1000L);
                 if (timeIdle > maxIdleSwap && timeIdle > minIdleSwap) {
                     if (debug > 1)
                         log(sm.getString
@@ -1036,15 +1061,16 @@
         long timeNow = System.currentTimeMillis();
 
         for (int i = 0; i < sessions.length && toswap > 0; i++) {
+            StandardSession session = (StandardSession)sessions[i];
             int timeIdle = // Truncate, do not round up
-                (int) ((timeNow - sessions[i].getLastAccessedTime()) / 1000L);
+                (int) ((timeNow - session.getLastUsedTime()) / 1000L);
             if (timeIdle > minIdleSwap) {
                 if(debug > 1)
                     log(sm.getString
                         ("persistentManager.swapTooManyActive",
-                         sessions[i].getId(), new Integer(timeIdle)));
+                         session.getId(), new Integer(timeIdle)));
                 try {
-                    swapOut(sessions[i]);
+                    swapOut(session);
                 } catch (IOException e) {
                     ;   // This is logged in writeSession()
                 }
@@ -1073,7 +1099,7 @@
                 if (!session.isValid())
                     continue;
                 int timeIdle = // Truncate, do not round up
-                    (int) ((timeNow - session.getLastAccessedTime()) / 1000L);
+                    (int) ((timeNow - session.getLastUsedTime()) / 1000L);
                 if (timeIdle > maxIdleBackup) {
                     if (debug > 1)
                         log(sm.getString
@@ -1160,6 +1186,7 @@
             threadSleep();
             processExpires();
             processPersistenceChecks();
+            sessionSwapIgnore.clear();
         }
 
     }
Index: StandardManager.java
===================================================================
RCS file: 
/home/cvs/jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/session/StandardManager.java,v
retrieving revision 1.21
diff -u -r1.21 StandardManager.java
--- StandardManager.java        6 Feb 2003 22:58:37 -0000       1.21
+++ StandardManager.java        20 Apr 2004 14:30:26 -0000
@@ -786,7 +786,7 @@
             if (maxInactiveInterval < 0)
                 continue;
             int timeIdle = // Truncate, do not round up
-                (int) ((timeNow - session.getLastAccessedTime()) / 1000L);
+                (int) ((timeNow - session.getLastUsedTime()) / 1000L);
             if (timeIdle >= maxInactiveInterval) {
                 try {
                     expiredSessions++;
Index: StandardSession.java
===================================================================
RCS file: 
/home/cvs/jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/session/StandardSession.java,v
retrieving revision 1.39
diff -u -r1.39 StandardSession.java
--- StandardSession.java        25 Mar 2004 22:15:45 -0000      1.39
+++ StandardSession.java        20 Apr 2004 14:30:26 -0000
@@ -300,7 +300,7 @@
     /**
      * The current accessed time for this session.
      */
-    private long thisAccessedTime = creationTime;
+    private long lastUsedTime = creationTime;
 
 
     // ----------------------------------------------------- Session Properties
@@ -342,7 +342,7 @@
 
         this.creationTime = time;
         this.lastAccessedTime = time;
-        this.thisAccessedTime = time;
+        this.lastUsedTime = time;
 
     }
 
@@ -444,6 +444,19 @@
 
 
     /**
+     * Return the last time a request was recieved associated with this
+     * session, as the number of milliseconds since midnight, January 1, 1970
+     * GMT.  Actions that your application takes, such as getting or setting
+     * a value associated with the session, do not affect the access time.
+     */
+    public long getLastUsedTime() {
+
+        return (this.lastUsedTime);
+
+    }
+
+
+    /**
      * Return the Manager within which this Session is valid.
      */
     public Manager getManager() {
@@ -579,8 +592,8 @@
     public void access() {
 
         this.isNew = false;
-        this.lastAccessedTime = this.thisAccessedTime;
-        this.thisAccessedTime = System.currentTimeMillis();
+        this.lastAccessedTime = this.lastUsedTime;
+        this.lastUsedTime = System.currentTimeMillis();
 
     }
 
@@ -772,6 +785,7 @@
         expiring = false;
         id = null;
         lastAccessedTime = 0L;
+        lastUsedTime = 0L;
         maxInactiveInterval = -1;
         notes.clear();
         setPrincipal(null);
@@ -1372,7 +1386,7 @@
         maxInactiveInterval = ((Integer) stream.readObject()).intValue();
         isNew = ((Boolean) stream.readObject()).booleanValue();
         isValid = ((Boolean) stream.readObject()).booleanValue();
-        thisAccessedTime = ((Long) stream.readObject()).longValue();
+        lastUsedTime = ((Long) stream.readObject()).longValue();
         principal = null;        // Transient only
         //        setId((String) stream.readObject());
         id = (String) stream.readObject();
@@ -1429,7 +1443,7 @@
         stream.writeObject(new Integer(maxInactiveInterval));
         stream.writeObject(new Boolean(isNew));
         stream.writeObject(new Boolean(isValid));
-        stream.writeObject(new Long(thisAccessedTime));
+        stream.writeObject(new Long(lastUsedTime));
         stream.writeObject(id);
         if (debug >= 2)
             log("writeObject() storing session " + id);
Index: StoreBase.java
===================================================================
RCS file: 
/home/cvs/jakarta-tomcat-4.0/catalina/src/share/org/apache/catalina/session/StoreBase.java,v
retrieving revision 1.7
diff -u -r1.7 StoreBase.java
--- StoreBase.java      4 Mar 2003 04:09:17 -0000       1.7
+++ StoreBase.java      20 Apr 2004 14:30:26 -0000
@@ -317,7 +317,7 @@
                     continue;
                 }
                 int timeIdle = // Truncate, do not round up
-                    (int) ((timeNow - session.getLastAccessedTime()) / 1000L);
+                    (int) ((timeNow - session.getLastUsedTime()) / 1000L);
                 if (timeIdle >= maxInactiveInterval) {
                     if ( ( (PersistentManagerBase) manager).isLoaded( keys[i] )) {
                         // recycle old backup session

---------------------------------------------------------------------
To unsubscribe, e-mail: [EMAIL PROTECTED]
For additional commands, e-mail: [EMAIL PROTECTED]

Reply via email to