Summary: | Double-check locking. Possible data-race in JspServletWrapper | ||
---|---|---|---|
Product: | Tomcat 6 | Reporter: | Sergey Vorobyev <sergeyvorobyev> |
Component: | Jasper | Assignee: | Tomcat Developers Mailing List <dev> |
Status: | RESOLVED FIXED | ||
Severity: | normal | CC: | konstantin.s.serebryany, sergeyvorobyev |
Priority: | P2 | ||
Version: | 6.0.29 | ||
Target Milestone: | default | ||
Hardware: | PC | ||
OS: | All | ||
Attachments: | Patch broken DCL |
Description
Sergey Vorobyev
2010-09-23 09:31:03 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. (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; } } } } 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). Created attachment 26102 [details]
Patch broken DCL
Fixed in trunk and will be in 7.0.4 onwards. Proposed for 6.0.x. Fixed in 6.0.x and will be in 6.0.30 onwards. |