Message-Id: <19990204214633.19077.qmail@hyperreal.org>
Date: 4 Feb 1999 21:46:33 -0000
From: Tom May <tom@go2net.com>
Reply-To: tom@go2net.com
To: apbugs@hyperreal.org
Subject: mod_jserv/3837: JServServletManager run() bugs
Sender: apache-bugdb-owner@apache.org
Precedence: bulk


>Number:         3837
>Category:       mod_jserv
>Synopsis:       JServServletManager run() bugs
>Confidential:   no
>Severity:       non-critical
>Priority:       medium
>Responsible:    jserv
>State:          open
>Class:          sw-bug
>Submitter-Id:   apache
>Arrival-Date:   Thu Feb  4 13:50:01 PST 1999
>Last-Modified:
>Originator:     tom@go2net.com
>Organization:
apache
>Release:        1.3.3
>Environment:
This report is for a generic coding error applicable to all systems.
>Description:
The JServServletManager (cvs rev 1.28) housekeeping thread has a number
of problems:
 
    /**
     * The housekeeping thread
     * Checks for sessions that have not been used for a certain
     * amount of time and invalidates them.
     */
    public void run() {
        Enumeration sesses;
        JServSession sess;
        long sysMillis;
 
        while(true) {
            // sleep for 5 seconds.
            // XXX: Make this configurable? (Vincent Partington)
            try {
                Thread.sleep(sessionCheckFrequency);
            } catch(InterruptedException exc) { }
 
            // walk through all sessions and invalidate old ones
            // FIXME: Should this be surrounded by synchronized(this)
            // to prevent race conditions? If there are a lot of sessions
            // the locking may cost a significant amount of time.
            sesses = sessions.elements();
            sysMillis = System.currentTimeMillis();
            while(sesses.hasMoreElements()) {
                sess = (JServSession) sesses.nextElement();
                if(sysMillis - sess.lastAccessTime > sessionTimeout) {
                    sess.invalidate();
                    break;
                }
            }
        }
    }
 
1. You should synchronize accesses to sess.lastAccessTime to avoid seeing
   a partial write to half of the 64-bit long value.  Synchronizing on
   sess will also eliminate obscure errors that can occur when other
   threads make modifications to sessions while you are enumerating it,
   which could otherwise cause newly created sessions to be immediately
   timed out and invalidated.
 
2. If the session has been invalidated before this code calls
   sess.invalidate() (which can happen as a race with some other thread
   calling invalidate()) then invalidate() will throw IllegalStateException
   which will cause the thread to exit since it isn't caught.
 
3. You shouldn't break after invalidating a single session.
 
>How-To-Repeat:

>Fix:
Try something like this:

            while(sesses.hasMoreElements()) {
                sess = (JServSession) sesses.nextElement();
                synchronized (sess) {
                    if(sysMillis - sess.lastAccessTime > sessionTimeout) {
                        try {
                            sess.invalidate();
                        }
                        catch (IllegalStateException ignored) {}
                    }
                }
            }
 
>Audit-Trail:
>Unformatted:
[In order for any reply to be added to the PR database, ]
[you need to include <apbugs@Apache.Org> in the Cc line ]
[and leave the subject line UNCHANGED.  This is not done]
[automatically because of the potential for mail loops. ]
[If you do not include this Cc, your reply may be ig-   ]
[nored unless you are responding to an explicit request ]
[from a developer.                                      ]
[Reply only with text; DO NOT SEND ATTACHMENTS!         ]