Remy Maucherat wrote:
Jess Holle wrote:
[Yes, the Request change would still be necessary to prevent the chance of requests randomly getting their sessions passivated and recycle after they findSession() but before access().]
A session can be passivated at any (bad) time (which will screw up requests), and we already do plenty of on access checks. I don't see what it adds.
A session must not be passivated at just *any* bad time -- that's part of what I'm fixing.
For the case where one has many valid sessions, many of which are passivated due to a maximum active sessions constraint, there is likely to be some level of thrashing such that it is entirely too likely that a session which has only been idle for a short while is getting passivated between the findSession() and the access() call. This will result in an otherwise fine request going horribly wrong from a user's perspective (assuming the state data in the session was at all critical).
If Request is left as is, then it user's will inappropriately get new sessions in such cases. If it is changed without the preAccess() addition, the user will get an invalidated session instead.
As I said, the patch ideas look fine, but I do have questions:
- Why do you really need methods named preAccess and access on the manager ? The first one seems to be designed to do swap ins while the second one keeps usage stats, so maybe this could be more elegant. Or maybe not.
The attached patch has some clarifying comments.
preAccess() is intended to fix the race condition case. It should rarely do more than check isValid and return, but without it user requests can go awry for no reason as they see it.
access() move the accessed session to the front of the LinkedHashMap thereby maintaining sorting. The attached patch also uses this sorting to reduce the overhead of processMaxIdleSwaps() by breaking from the loop as newer sessions are encountered. [Yes, I know this may mean some old sessions are not passivated when a session access on a very old session occurs between findSessions() and this check, but that's better than running through all sessions every processing interval. processMaxIdleSwaps() should be a best reasonable effort.]
- Keeping session skeletons around will eat resources. I understand this will improve performance, but is that really a good idea ? I'm asking for your experience after the change here (is memory usage much higher, etc).
I am working all of this on 2 fronts here: trying to get an acceptable patch together, incorporate any feedback, etc, and get this tested against real world scenarios where we're having issues.
Our issue involves various web apps that keep large amounts of session data for all users -- and a corresponding inability to scale beyond a few users. I am certain all of these changes put together will not suffice to totally fix this problem -- and efforts to improve the web apps are therefore also planned.
I'll certainly provide feedback (and further patches if/where appropriate) as I gain more real world experience with this approach.
-- Jess Holle
Index: PersistentManagerBase.java
===================================================================
RCS file:
/home/cvspublic/jakarta-tomcat-catalina/catalina/src/share/org/apache/catalina/session/PersistentManagerBase.java,v
retrieving revision 1.25
diff -u -r1.25 PersistentManagerBase.java
--- PersistentManagerBase.java 22 Nov 2004 16:35:18 -0000 1.25
+++ PersistentManagerBase.java 17 Dec 2004 15:41:30 -0000
@@ -20,6 +20,11 @@
import java.beans.PropertyChangeEvent;
import java.beans.PropertyChangeListener;
import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.LinkedHashMap;
+import java.util.Map;
import java.security.AccessController;
import java.security.PrivilegedActionException;
import java.security.PrivilegedExceptionAction;
@@ -204,17 +209,44 @@
protected long processingTime = 0;
- // ------------------------------------------------------------- Properties
+ /**
+ * Whether session should be synchronously swapped out immediately upon
+ * exceeding maxActiveSessions. Defaults to 'false'.
+ */
+ protected boolean immediateSwapOnMaxActiveExceeded = false;
+
+
+ // -------------------------------------------------------------
Constructors
+
+
+ public PersistentManagerBase() {
+ sessions = new LinkedHashMap() {
+ protected boolean removeEldestEntry(Map.Entry eldest) {
+ if ( immediateSwapOnMaxActiveExceeded ) {
+ int maxActiveSessions = getMaxActiveSessions();
+ if ( maxActiveSessions >= 0 )
+ if ( size() > maxActiveSessions )
+ if ( isStarted() )
+ // try to swap out oldest entry; if entry is
still too fresh then processMaxActiveSwaps() will swap out sessions when
possible in the background
+ swapOutSessionIfOldEnough( (StandardSession)
eldest.getValue(), System.currentTimeMillis(), minIdleSwap,
"persistentManager.swapTooManyActive" );
+ }
+ return ( false );
+ }
+ };
+ }
-
+ // ------------------------------------------------------------- Properties
+
+
+
/**
- * Indicates how many seconds old a session can get, after its last use
in a
- * request, before it should be backed up to the store. -1 means
sessions
- * are not backed up.
- */
+ * Indicates how many seconds old a session can get, after its last use in
a
+ * request, before it should be backed up to the store. -1 means sessions
+ * are not backed up.
+ */
public int getMaxIdleBackup() {
return maxIdleBackup;
@@ -314,13 +346,13 @@
/**
- * Set the Container with which this Manager has been associated. If it
is a
- * Context (the usual case), listen for changes to the session timeout
- * property.
- *
- * @param container
- * The associated Container
- */
+ * Set the Container with which this Manager has been associated. If it is
a
+ * Context (the usual case), listen for changes to the session timeout
+ * property.
+ *
+ * @param container
+ * The associated Container
+ */
public void setContainer(Container container) {
// De-register from the old Container (if any)
@@ -500,6 +532,23 @@
}
+ public boolean isImmediateSwapOnMaxActiveExceeded() {
+ return immediateSwapOnMaxActiveExceeded;
+ }
+
+
+
+ public void setImmediateSwapOnMaxActiveExceeded( boolean
immediateSwapOnMaxActiveExceeded ) {
+ if ( this.immediateSwapOnMaxActiveExceeded ==
immediateSwapOnMaxActiveExceeded )
+ return;
+ boolean oldImmediateSwapOnMaxActiveExceeded =
this.immediateSwapOnMaxActiveExceeded ;
+ this.immediateSwapOnMaxActiveExceeded =
immediateSwapOnMaxActiveExceeded ;
+ support.firePropertyChange("immediateSwapOnMaxActiveExceeded ",
+ new
Boolean(oldImmediateSwapOnMaxActiveExceeded ),
+ new
Boolean(this.immediateSwapOnMaxActiveExceeded ));
+ }
+
+
// --------------------------------------------------------- Public Methods
@@ -534,8 +583,8 @@
/**
* Implements the Manager interface, direct call to processExpires and
processPersistenceChecks
*/
- public void processExpires() {
-
+ public void processExpires() {
+
long timeNow = System.currentTimeMillis();
Session sessions[] = findSessions();
int expireHere = 0 ;
@@ -556,8 +605,8 @@
if(log.isDebugEnabled())
log.debug("End expire sessions " + getName() + " processingTime "
+ (timeEnd - timeNow) + " expired sessions: " + expireHere);
processingTime += (timeEnd - timeNow);
-
- }
+
+ }
/**
@@ -609,6 +658,62 @@
}
/**
+ * Get new session class to be used in the doLoad() method.
+ */
+ protected StandardSession getNewSession() {
+ return new PersistentSession(this);
+ }
+
+ /** If upon accessing a session we find it initially invalid see if it was
+ * just swapped out. If so, attempt to read the data back into this
+ * session object as someone got a copy prior to the swapOut.
+ *
+ * @param session Session that is being accessed.
+ */
+ protected void preAccess( Session session )
+ {
+ PersistentSession persistentSession = (PersistentSession) session;
+ if ( persistentSession.isValid || persistentSession.expiring )
+ return; // session is currently valid (but maybe out of date)
+
+ // if session is invalid it may have been passivated between
findSession() and access()!
+ StandardSession savedSession = null;
+ String id = persistentSession.getId();
+ try
+ {
+ savedSession = (StandardSession) swapInWithoutAdd( id );
+ }
+ catch ( IOException e )
+ {
+ if (log.isDebugEnabled())
+ log.debug("Load from store failed for " + id );
+ }
+ if ( savedSession == null )
+ return; // no passivated/saved session
+
+ // reactivate session
+ persistentSession.copyFieldsOf( savedSession );
+ addSwappedInSession( persistentSession );
+ savedSession.recycle();
+ }
+
+ /**
+ * Signal to Manager that session has been accessed.
+ * <p>
+ * This is used here to maintain MRU/LRU data on sessions.
+ *
+ * @param session Session that is being accessed.
+ */
+ protected void access( Session session ) {
+ synchronized (sessions) {
+ // pull session out of map and put it back in, thus sorting it
+ String sid = session.getId();
+ sessions.remove(sid);
+ sessions.put(sid, session);
+ }
+ }
+
+ /**
* Load all sessions found in the persistence mechanism, assuming
* they are marked as valid and have not passed their expiration
* limit. If persistence is not supported, this method returns
@@ -660,6 +765,39 @@
log.error("Failed load session from store, " + e.getMessage(),
e);
}
+ sortSessionsByAccessTimes();
+ }
+
+
+ /** Sort sessions by access times, i.e. during load().
+ */
+ private void sortSessionsByAccessTimes() {
+ ArrayList sessionList;
+ {
+ Session sessionArr[] = findSessions();
+ sessionList = new ArrayList( sessionArr.length );
+ for ( int i = 0; i < sessionArr.length; ++i )
+ sessionList.add( sessionArr[i] );
+ }
+ Collections.sort( sessionList,
+ new Comparator() {
+ public int compare(Object o1, Object o2) {
+ StandardSession session1 = (StandardSession) o1;
+ StandardSession session2 = (StandardSession) o2;
+ long accessTime1 = session1.thisAccessedTime;
+ long accessTime2 = session2.thisAccessedTime;
+ if ( accessTime1 < accessTime2 )
+ return -1;
+ if ( accessTime1 == accessTime2 )
+ return 0;
+ // if ( accessTime1 > accessTime2 )
+ return 1;
+ }
+ }
+ );
+ int nSessions = sessionList.size();
+ for ( int i = 0; i < nSessions; ++i )
+ access( (Session) sessionList.get( i ) );
}
@@ -749,6 +887,13 @@
* is invalid or past its expiration.
*/
protected Session swapIn(String id) throws IOException {
+ Session session = swapInWithoutAdd( id );
+ if ( session != null )
+ addSwappedInSession( session );
+ return ( session );
+ }
+
+ protected Session swapInWithoutAdd(String id) throws IOException {
if (store == null)
return null;
@@ -791,15 +936,19 @@
if(log.isDebugEnabled())
log.debug(sm.getString("persistentManager.swapIn", id));
+ return (session);
+
+ }
+
+ protected void addSwappedInSession( Session session )
+ {
+ StandardSession standardSession = (StandardSession) session;
session.setManager(this);
// make sure the listeners know about it.
- ((StandardSession)session).tellNew();
+ standardSession.tellNew();
add(session);
- ((StandardSession)session).activate();
- session.endAccess();
-
- return (session);
-
+ standardSession.activate();
+ standardSession.setNew( false );
}
@@ -1023,29 +1172,11 @@
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.getId(), new Integer(timeIdle)));
- try {
- swapOut(session);
- } catch (IOException e) {
- ; // This is logged in writeSession()
- }
- }
- }
- }
-
+ int minSwapSecs = ( ( maxIdleSwap > minIdleSwap ) ? maxIdleSwap :
minIdleSwap );
+ for (int i = sessions.length - 1; i >= 0; --i)
+ if ( swapOutSessionIfOldEnough( (StandardSession) sessions[i],
timeNow, minSwapSecs, "persistentManager.swapMaxIdle" ) ==
SESSION_NOT_OLD_ENOUGH )
+ break; // assume sorting hold well enough; don't bother
checking rest of sessions
+
}
@@ -1059,7 +1190,6 @@
Session sessions[] = findSessions();
- // FIXME: Smarter algorithm (LRU)
if (getMaxActiveSessions() >= sessions.length)
return;
@@ -1071,24 +1201,44 @@
int toswap = sessions.length - getMaxActiveSessions();
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].getId(), new Integer(timeIdle)));
- try {
- swapOut(sessions[i]);
- } catch (IOException e) {
- ; // This is logged in writeSession()
- }
- toswap--;
- }
- }
+ for (int i = sessions.length - 1; i >= 0 && toswap > 0; --i)
+ if ( swapOutSessionIfOldEnough( (StandardSession) sessions[i],
timeNow, minIdleSwap, "persistentManager.swapTooManyActive" ) ==
SESSION_SWAPPED_OUT_OK )
+ --toswap;
}
+
+
+ private static final int SESSION_NOT_OLD_ENOUGH = 1;
+ private static final int SESSION_BEING_ACCESSED = 2;
+ private static final int SESSION_SWAPPED_OUT_OK = 3;
+
+ /**
+ * Returns -1 if session not old enough, 1 if still being accessed, and 0
if swapped out
+ */
+ private int swapOutSessionIfOldEnough( StandardSession session, long
timeNow, int minSwapSecs, String debugString ) {
+ synchronized ( session ) {
+ int timeIdle = // Truncate, do not round up
+ (int) ((timeNow - session.thisAccessedTime) / 1000L);
+ if (timeIdle > minSwapSecs ) {
+ if ( session.accessCount > 0 ) {
+ if (log.isDebugEnabled())
+ log.debug(
"PersistentManagerBase.swapOutSessionIfOldEnough(): session [" +
session.getId() +
+ "] access count [" + session.accessCount + "]
> 0 " );
+ return ( SESSION_BEING_ACCESSED ); // session may still be
being accessed
+ }
+ if(log.isDebugEnabled())
+ log.debug(sm.getString(debugString, session.getId(), new
Integer(timeIdle)));
+ try {
+ swapOut(session);
+ } catch (IOException e) {
+ ; // This is logged in writeSession()
+ }
+ return ( SESSION_SWAPPED_OUT_OK );
+ }
+ else
+ return ( SESSION_NOT_OLD_ENOUGH );
+ }
+ }
/**
@@ -1104,22 +1254,32 @@
// Back up all sessions idle longer than maxIdleBackup
if (maxIdleBackup >= 0) {
- for (int i = 0; i < sessions.length; i++) {
+ for (int i = sessions.length - 1; i >= 0; --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.getId(), 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.thisAccessedTime) / 1000L);
+ if (timeIdle > maxIdleBackup) {
+ if ( session.accessCount > 0 ) {
+ if (log.isDebugEnabled())
+ log.debug(
"PersistentManagerBase.processMaxIdleBackups(): session [" + session.getId() +
+ "] access count [" +
session.accessCount + "] > 0 " );
+ continue; // session may still be being accessed
+ }
+
+ // TO_DO: enhance store to allow quick
'if-modified-since' sort of check here!
+ if (log.isDebugEnabled())
+ log.debug(sm.getString
+ ("persistentManager.backupMaxIdle",
+ session.getId(), new Integer(timeIdle)));
+ try {
+ writeSession(session);
+ } catch (IOException e) {
+ ; // This is logged in writeSession()
+ }
}
}
}--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
