On 01/07/2009, ma...@apache.org <ma...@apache.org> wrote: > Author: markt > Date: Wed Jul 1 13:19:15 2009 > New Revision: 790155 > > URL: http://svn.apache.org/viewvc?rev=790155&view=rev > Log: > Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=43343 > Port of r656751, r778523, r778524, r784453 and r784602 > Because, unlike 6.0.x, accessCount is not atomic I made it volatile and > sync'd the updates. > > Modified: > > tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/PersistentManagerBase.java > > tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/StandardSession.java > > tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/valves/PersistentValve.java > tomcat/container/tc5.5.x/webapps/docs/changelog.xml > tomcat/container/tc5.5.x/webapps/docs/config/manager.xml > > Modified: > tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/PersistentManagerBase.java > URL: > http://svn.apache.org/viewvc/tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/PersistentManagerBase.java?rev=790155&r1=790154&r2=790155&view=diff > > ============================================================================== > --- > tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/PersistentManagerBase.java > (original) > +++ > tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/PersistentManagerBase.java > Wed Jul 1 13:19:15 2009 > @@ -590,6 +590,23 @@ > 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());
Not sure this will work correctly, as you are changing the lock in the middle of a synch. block. This looks very much like another instance of https://issues.apache.org/bugzilla/show_bug.cgi?id=46990 > + if(session != null){ > + // To keep any external calling code from messing up the > + // concurrency. > + session.access(); > + session.endAccess(); > + } > + } > + } > if (session != null) > return (session); > > @@ -797,6 +814,9 @@ > ((StandardSession)session).tellNew(); > add(session); > ((StandardSession)session).activate(); > + // endAccess() to ensure timeouts happen correctly. > + // access() to keep access count correct or it will end up negative > + session.access(); > session.endAccess(); > > return (session); > @@ -1023,24 +1043,28 @@ > 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 (session.accessCount > 0) { > + // Session is currently being accessed - skip it > + continue; > + } > + 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() > + } > } > } > } > @@ -1072,19 +1096,26 @@ > 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() > + StandardSession session = (StandardSession) sessions[i]; > + synchronized (session) { > + int timeIdle = // Truncate, do not round up > + (int) ((timeNow - session.getLastAccessedTime()) / > 1000L); > + if (timeIdle > minIdleSwap) { > + if (session.accessCount > 0) { > + // Session is currently being accessed - skip it > + continue; > + } > + if(log.isDebugEnabled()) > + log.debug(sm.getString > + ("persistentManager.swapTooManyActive", > + session.getIdInternal(), new > Integer(timeIdle))); > + try { > + swapOut(session); > + } catch (IOException e) { > + // This is logged in writeSession() > + } > + toswap--; > } > - toswap--; > } > } > > @@ -1106,20 +1137,22 @@ > 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() > + } > } > } > } > > Modified: > tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/StandardSession.java > URL: > http://svn.apache.org/viewvc/tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/StandardSession.java?rev=790155&r1=790154&r2=790155&view=diff > > ============================================================================== > --- > tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/StandardSession.java > (original) > +++ > tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/session/StandardSession.java > Wed Jul 1 13:19:15 2009 > @@ -273,7 +273,7 @@ > /** > * The access count for this session. > */ > - protected transient int accessCount = 0; > + protected volatile transient int accessCount = 0; > > private Object lock = new Object(); > > @@ -711,7 +711,9 @@ > } > } > } > - accessCount = 0; > + synchronized(lock) { > + accessCount = 0; > + } > setValid(false); > > /* > @@ -851,7 +853,9 @@ > id = null; > lastAccessedTime = 0L; > maxInactiveInterval = -1; > - accessCount = 0; > + synchronized(lock) { > + accessCount = 0; > + } > notes.clear(); > setPrincipal(null); > isNew = false; > > Modified: > tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/valves/PersistentValve.java > URL: > http://svn.apache.org/viewvc/tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/valves/PersistentValve.java?rev=790155&r1=790154&r2=790155&view=diff > > ============================================================================== > --- > tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/valves/PersistentValve.java > (original) > +++ > tomcat/container/tc5.5.x/catalina/src/share/org/apache/catalina/valves/PersistentValve.java > Wed Jul 1 13:19:15 2009 > @@ -35,10 +35,13 @@ > > > /** > - * 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$ > @@ -134,6 +137,7 @@ > manager.add(session); > // ((StandardSession)session).activate(); > session.access(); > + session.endAccess(); > } > } > } > > Modified: tomcat/container/tc5.5.x/webapps/docs/changelog.xml > URL: > http://svn.apache.org/viewvc/tomcat/container/tc5.5.x/webapps/docs/changelog.xml?rev=790155&r1=790154&r2=790155&view=diff > > ============================================================================== > --- tomcat/container/tc5.5.x/webapps/docs/changelog.xml (original) > +++ tomcat/container/tc5.5.x/webapps/docs/changelog.xml Wed Jul 1 13:19:15 > 2009 > @@ -96,6 +96,10 @@ > <bug>42707</bug>: Make adding a host alias via JMX take effect > immediately. (markt) > </fix> > + <fix> > + <bug>43343</bug>: Correctly handle requesting a session we are in > the > + middle of persisting. Based on a suggestion by Wade Chandler. > (markt) > + </fix> > <add> > <bug>44382</bug>: Add support for using httpOnly for session > cookies. > This is disabled by default. (markt/fhanik) > > Modified: tomcat/container/tc5.5.x/webapps/docs/config/manager.xml > URL: > http://svn.apache.org/viewvc/tomcat/container/tc5.5.x/webapps/docs/config/manager.xml?rev=790155&r1=790154&r2=790155&view=diff > > ============================================================================== > --- tomcat/container/tc5.5.x/webapps/docs/config/manager.xml (original) > +++ tomcat/container/tc5.5.x/webapps/docs/config/manager.xml Wed Jul 1 > 13:19:15 2009 > @@ -164,7 +164,12 @@ > <p><em><strong>WARNING - Use of this Manager implementation > has not been thoroughly tested, and should be considered experimental! > </strong></em></p> > - > + > + <p><strong>NOTE:</strong> You must set the > + <code>org.apache.catalina.STRICT_SERVLET_COMPLIANCE</code> > + <a href="systemprops.html">system property</a> to <code>true</code> for > + the persistent manager to work correctly.</p> > + > <p>The persistent implementation of <strong>Manager</strong> is > <strong>org.apache.catalina.session.PersistentManager</strong>. In > addition to the usual operations of creating and deleting sessions, a > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
