Bug 49986 - Double-check locking. Possible data-race in JspServletWrapper
Double-check locking. Possible data-race in JspServletWrapper
Status: RESOLVED FIXED
Product: Tomcat 6
Classification: Unclassified
Component: Jasper
6.0.29
PC All
: P2 normal (vote)
: default
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2010-09-23 09:31 UTC by Sergey Vorobyev
Modified: 2010-10-13 10:59 UTC (History)
2 users (show)



Attachments
Patch broken DCL (2.39 KB, patch)
2010-09-30 01:30 UTC, Tim Whittington
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergey Vorobyev 2010-09-23 09:31:03 UTC
r998053
http://svn.apache.org/repos/asf/tomcat/tc6.0.x/trunk/java/org/apache/jasper/servlet/JspServletWrapper.java

Common anti-pattern: Double checked locking. I don't know is this race dangerous or no.

Race on private boolean reload
    
public Servlet getServlet()
        throws ServletException, IOException, FileNotFoundException
    {
        if (reload) {
            synchronized (this) {
                // Synchronizing on jsw enables simultaneous loading
                // of different pages, but not the same page.
                if (reload) {
                    // This is to maintain the original protocol.
                   
..........

                    theServlet = servlet;
                    reload = false;
                }
            }    
        }
        return theServlet;
    }
Comment 1 Sebb 2010-09-23 10:18:53 UTC
Looks like a problem to me if the class can be called from multiple threads.
If not, then why have any synch. blocks at all?

Could perhaps be fixed by making reload volatile, but the reload variable can be set false elsewhere without synch.

The servletClassLastModifiedTime also looks to be used in a similar way; that is a long so could potentially an invalid value could be read.
Comment 2 Sergey Vorobyev 2010-09-23 10:29:38 UTC
(In reply to comment #1)
> Looks like a problem to me if the class can be called from multiple threads.
> If not, then why have any synch. blocks at all?

It's called from multiple threads.
Stack trace:
    Thread 28
    #0  org/apache/jasper/servlet/JspServletWrapper.getServlet JspServletWrapper.java:166
    #1  org/apache/jasper/servlet/JspServletWrapper.service JspServletWrapper.java:329
    #2  org/apache/jasper/servlet/JspServlet.serviceJspFile JspServlet.java:313
    #3  org/apache/jasper/servlet/JspServlet.service JspServlet.java:260
    #4  javax/servlet/http/HttpServlet.service HttpServlet.java:717
    #5  org/apache/catalina/core/ApplicationFilterChain.internalDoFilter ApplicationFilterChain.java:290
    #6  org/apache/catalina/core/ApplicationFilterChain.doFilter ApplicationFilterChain.java:206
    #7  org/apache/catalina/core/StandardWrapperValve.invoke StandardWrapperValve.java:233
    #8  org/apache/catalina/core/StandardContextValve.invoke StandardContextValve.java:191
    #9  org/apache/catalina/authenticator/AuthenticatorBase.invoke AuthenticatorBase.java:470
    #10 org/apache/catalina/core/StandardHostValve.invoke StandardHostValve.java:127
    #11 org/apache/catalina/valves/ErrorReportValve.invoke ErrorReportValve.java:102
 
   Thread 25
    #0  org/apache/jasper/servlet/JspServletWrapper.getServlet JspServletWrapper.java:133
    #1  org/apache/jasper/servlet/JspServletWrapper.service JspServletWrapper.java:329
    #2  org/apache/jasper/servlet/JspServlet.serviceJspFile JspServlet.java:313
    #3  org/apache/jasper/servlet/JspServlet.service JspServlet.java:260
    #4  javax/servlet/http/HttpServlet.service HttpServlet.java:717
    #5  org/apache/catalina/core/ApplicationFilterChain.internalDoFilter ApplicationFilterChain.java:290
    #6  org/apache/catalina/core/ApplicationFilterChain.doFilter ApplicationFilterChain.java:206
    #7  org/apache/catalina/core/StandardWrapperValve.invoke StandardWrapperValve.java:233
    #8  org/apache/catalina/core/StandardContextValve.invoke StandardContextValve.java:191
    #9  org/apache/catalina/authenticator/AuthenticatorBase.invoke AuthenticatorBase.java:470

> The servletClassLastModifiedTime also looks to be used in a similar way; that
> is a long so could potentially an invalid value could be read.

Exactly! Thank you for point    

     public void setServletClassLastModifiedTime(long lastModified) {
        if (this.servletClassLastModifiedTime < lastModified) {
            synchronized (this) {
                if (this.servletClassLastModifiedTime < lastModified) {
                    this.servletClassLastModifiedTime = lastModified;
                    reload = true;
                }
            }
        }
    }
Comment 3 Tim Whittington 2010-09-30 01:27:22 UTC
This is actually one of the rare cases that DCL is a decent solution, but the current implementation is broken.

The fix for getServlet (now Java 5 has come along) is a volatile reload flag.
This forces an in order write of the new servlet object and the 'theServlet' reference (as long as the write to reload is the last step in the update), as well as forcing a read barrier (and thus a consistent read of 'theServlet' and the new servlet object) for any thread entering getServlet after a reload is done.

setServletClassLastModifiedTime can be fixed by making servletClassLastModifiedTime volatile (again since Java 5).
(This could be done with an AtomicLong and a busy loop compareAndSet, but the volatile DCL is a more minor change and the performance diff is probably negligible).
e.g.:
        final AtomicLong lastModifiedTime = new AtomicLong();

        while (true) {
            long current = lastModifiedTime.get();
            if (current < lastModified) {
                if (lastModifiedTime.compareAndSet(current, lastModified)) {
                    reload = true;
                    break;
                }
            } else {
                break;
            }
        }

I'll move this to Tomcat 7 and look at a fix there - depending on the confidence we have it may find it's way back to 6.x (although I've been running Tomcat in production for a decade and have never seen a practical failure from this).
Comment 4 Tim Whittington 2010-09-30 01:30:16 UTC
Created attachment 26102 [details]
Patch broken DCL
Comment 5 Mark Thomas 2010-10-07 10:37:07 UTC
Fixed in trunk and will be in 7.0.4 onwards.

Proposed for 6.0.x.
Comment 6 Mark Thomas 2010-10-13 10:59:02 UTC
Fixed in 6.0.x and will be in 6.0.30 onwards.