tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r790155 - in /tomcat/container/tc5.5.x: catalina/src/share/org/apache/catalina/session/ catalina/src/share/org/apache/catalina/valves/ webapps/docs/ webapps/docs/config/
Date Wed, 01 Jul 2009 19:36:23 GMT
On 01/07/2009, markt@apache.org <markt@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-unsubscribe@tomcat.apache.org
>  For additional commands, e-mail: dev-help@tomcat.apache.org
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message