Bug 40380 - Potential syncro problem in StandardSession.expire(boolean)
Potential syncro problem in StandardSession.expire(boolean)
Status: RESOLVED FIXED
Product: Tomcat 5
Classification: Unclassified
Component: Servlet & JSP API
5.5.5
All other
: P2 normal (vote)
: ---
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2006-08-31 18:57 UTC by Claude Qu
Modified: 2009-07-17 04:16 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
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.