Bug 40380

Summary: Potential syncro problem in StandardSession.expire(boolean)
Product: Tomcat 5 Reporter: Claude Qu <cquezel>
Component: Servlet & JSP APIAssignee: Tomcat Developers Mailing List <dev>
Status: RESOLVED FIXED    
Severity: normal    
Priority: P2    
Version: 5.5.5   
Target Milestone: ---   
Hardware: All   
OS: other   

Description Claude Qu 2006-08-31 18:57:30 UTC
public void expire(boolean notify) {

        // Mark this session as "being expired" if needed
        if (expiring)
            return;

// No man's land here

        synchronized (this) {

            if (manager == null)
                return;

            expiring = true;
Comment 1 Yoav Shapira 2006-12-25 05:43:08 UTC
Have you run into any issues with this, or was it just a random code inspection?
Comment 2 Darryl Miles 2006-12-27 01:42:26 UTC
Just to butt in here.  

The easiest fix is to move or copy the if(expiring) inside the synchronized()
section.

Does such a large block have to be synchronized(this) {} ?  I can't see any
other code using synchronized(this) in the class (or synchronized(session) in
the package), so I guess the only thing being protected is the
if(expiring==false) { expiring=true; } test and set.


As things stand now is the StandardSession author sure there is no issue of
needing to make the JVM perform a memory write barrier ?  To ensure that
expiring=true is flushed for other threads see it immediately.  Otherwise the
JVM maybe free to optimize (and defer) this write to memory (unless you start
getting into declaring 'expiring' volatile).


Suggestion:

if (expiring)
    return;

synchronized (this) {
    if (expiring)
        return;
    if (manager == null)
        return;
    expiring = true;
}

// From here is no need to keep the lock AFAIKS and by closing the
synchronized() we will ensure memory write barrier.

// Towards the end of the function it sets expiring=false; I dont think the any
code in critical to seeing the expiring==false again, not once isValid==false in
anycase.
Comment 3 Mark Thomas 2009-07-14 10:36:52 UTC
This has been fixed in trunk and proposed for 6.0.x and 5.5.x
Comment 4 Mark Thomas 2009-07-17 04:16:21 UTC
This has been fixed in 6.0.x and 5.5.x and will be included in 6.0.21 and 5.5.28 onwards.