r998053 http://svn.apache.org/repos/asf/tomcat/tc6.0.x/trunk/java/org/apache/el/parser/AstInteger.java http://svn.apache.org/repos/asf/tomcat/tc6.0.x/trunk/java/org/apache/el/parser/AstFloatingPoint.java http://svn.apache.org/repos/asf/tomcat/tc6.0.x/trunk/java/org/apache/el/parser/AstString.java Use lazy init without any synchronization in methods getInteger(), getFloatingPoint(), getString(), respectively. Consider AstInteger: private Number number; protected Number getInteger() { if (this.number == null) { try { this.number = new Long(this.image); } catch (ArithmeticException e1) { this.number = new BigInteger(this.image); } } return number; } Data races on variable number :37 in method getInteger() :39 concurrent read on line 40 concurrent write on lines 42, 44
I don't see any way this could cause a problem. If you have a scenario where this could lead to unexpected behaviour, feel free to re-open this issue and add an explanation.
First and most obviously, two or more Long/BigInteger objects could be allocated. It's not dangerous, but not good. Second consider following: Thread1: protected Number getInteger() { if (this.number == null) { try { -> this.number = new Long(this.image); // this.number isn't null, but not fully complete. Thread2: protected Number getInteger() { if (this.number == null) { // break, because this.number isn't null already. ... } -> return number; // publish not fully complete object link. } For more reasons see http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html (It's about dcl, but about lazy initialization generally two. First example become to this code exactly)
AIUI making "number" volatile would prevent the re-ordering that can result in the object reference being set up before construction is complete. Yes?
(In reply to comment #3) > AIUI making "number" volatile would prevent the re-ordering that can result in > the object reference being set up before construction is complete. Yes? Yes, the reason is object reference being set up before construction is complete. One of possible solutions is making "number" volatile.
I've yet to be convinced that there is an real issue here. I don't see any multi-threaded code paths into instances of those classes. If we change anything (on the basis there might be a way for multiple threads to use one of these objects at the same time) then using volatile should prevent misbehaviour with minimal impact. I'm not concerned about possible additional object creation.
Lock at this stack-traces to see how this method can be on invoked from a number of threads: (I'm using dacapo tomcat benchmark) AstInteger: Thread 22 #0 org/apache/el/parser/AstInteger.getInteger AstInteger.java:42 #1 org/apache/el/parser/AstInteger.getValue AstInteger.java:57 #2 org/apache/el/parser/AstDiv.getValue AstDiv.java:39 #3 org/apache/el/parser/AstGreaterThan.getValue AstGreaterThan.java:41 #4 org/apache/el/ValueExpressionImpl.getValue ValueExpressionImpl.java:186 #5 org/apache/jasper/runtime/PageContextImpl.proprietaryEvaluate PageContextImpl.java:935 #6 org/apache/jsp/jsp/jsp2/el/basic_002dcomparisons_jsp._jspService basic_002dcomparisons_jsp.java:115 #7 org/apache/jasper/runtime/HttpJspBase.service HttpJspBase.java:70 #8 javax/servlet/http/HttpServlet.service HttpServlet.java:717 #9 org/apache/jasper/servlet/JspServletWrapper.service JspServletWrapper.java:377 #10 org/apache/jasper/servlet/JspServlet.serviceJspFile JspServlet.java:313 #11 org/apache/jasper/servlet/JspServlet.service JspServlet.java:260 Thread 28 #0 org/apache/el/parser/AstInteger.getInteger AstInteger.java:40 #1 org/apache/el/parser/AstInteger.getValue AstInteger.java:57 #2 org/apache/el/parser/AstDiv.getValue AstDiv.java:39 #3 org/apache/el/parser/AstGreaterThan.getValue AstGreaterThan.java:41 #4 org/apache/el/ValueExpressionImpl.getValue ValueExpressionImpl.java:186 #5 org/apache/jasper/runtime/PageContextImpl.proprietaryEvaluate PageContextImpl.java:935 #6 org/apache/jsp/jsp/jsp2/el/basic_002dcomparisons_jsp._jspService basic_002dcomparisons_jsp.java:115 #7 org/apache/jasper/runtime/HttpJspBase.service HttpJspBase.java:70 #8 javax/servlet/http/HttpServlet.service HttpServlet.java:717 #9 org/apache/jasper/servlet/JspServletWrapper.service JspServletWrapper.java:377 AstFloatingPoint: Thread 22 #0 org/apache/el/parser/AstFloatingPoint.getFloatingPoint AstFloatingPoint.java:40 #1 org/apache/el/parser/AstFloatingPoint.getValue AstFloatingPoint.java:52 #2 org/apache/el/parser/AstGreaterThanEqual.getValue AstGreaterThanEqual.java:37 #3 org/apache/el/ValueExpressionImpl.getValue ValueExpressionImpl.java:186 #4 org/apache/jasper/runtime/PageContextImpl.proprietaryEvaluate PageContextImpl.java:935 #5 org/apache/jsp/jsp/jsp2/el/basic_002dcomparisons_jsp._jspService basic_002dcomparisons_jsp.java:121 #6 org/apache/jasper/runtime/HttpJspBase.service HttpJspBase.java:70 #7 javax/servlet/http/HttpServlet.service HttpServlet.java:717 #8 org/apache/jasper/servlet/JspServletWrapper.service JspServletWrapper.java:377 #9 org/apache/jasper/servlet/JspServlet.serviceJspFile JspServlet.java:313 #10 org/apache/jasper/servlet/JspServlet.service JspServlet.java:260 #11 javax/servlet/http/HttpServlet.service HttpServlet.java:717 Thread 28 #0 org/apache/el/parser/AstFloatingPoint.getFloatingPoint AstFloatingPoint.java:42 #1 org/apache/el/parser/AstFloatingPoint.getValue AstFloatingPoint.java:52 #2 org/apache/el/parser/AstGreaterThanEqual.getValue AstGreaterThanEqual.java:37 #3 org/apache/el/ValueExpressionImpl.getValue ValueExpressionImpl.java:186 #4 org/apache/jasper/runtime/PageContextImpl.proprietaryEvaluate PageContextImpl.java:935 #5 org/apache/jsp/jsp/jsp2/el/basic_002dcomparisons_jsp._jspService basic_002dcomparisons_jsp.java:121 #6 org/apache/jasper/runtime/HttpJspBase.service HttpJspBase.java:70 #7 javax/servlet/http/HttpServlet.service HttpServlet.java:717 #8 org/apache/jasper/servlet/JspServletWrapper.service JspServletWrapper.java:377 #9 org/apache/jasper/servlet/JspServlet.serviceJspFile JspServlet.java:313 AstString: Thread 22 #0 org/apache/el/parser/AstString.getString AstString.java:38 #1 org/apache/el/parser/AstString.getValue AstString.java:51 #2 org/apache/el/parser/AstLessThan.getValue AstLessThan.java:37 #3 org/apache/el/ValueExpressionImpl.getValue ValueExpressionImpl.java:186 #4 org/apache/jasper/runtime/PageContextImpl.proprietaryEvaluate PageContextImpl.java:935 #5 org/apache/jsp/jsp/jsp2/el/basic_002dcomparisons_jsp._jspService basic_002dcomparisons_jsp.java:179 #6 org/apache/jasper/runtime/HttpJspBase.service HttpJspBase.java:70 #7 javax/servlet/http/HttpServlet.service HttpServlet.java:717 #8 org/apache/jasper/servlet/JspServletWrapper.service JspServletWrapper.java:377 #9 org/apache/jasper/servlet/JspServlet.serviceJspFile JspServlet.java:313 #10 org/apache/jasper/servlet/JspServlet.service JspServlet.java:260 #11 javax/servlet/http/HttpServlet.service HttpServlet.java:717 Thread 28 #0 org/apache/el/parser/AstString.getString AstString.java:39 #1 org/apache/el/parser/AstString.getValue AstString.java:51 #2 org/apache/el/parser/AstLessThan.getValue AstLessThan.java:37 #3 org/apache/el/ValueExpressionImpl.getValue ValueExpressionImpl.java:186 #4 org/apache/jasper/runtime/PageContextImpl.proprietaryEvaluate PageContextImpl.java:935 #5 org/apache/jsp/jsp/jsp2/el/basic_002dcomparisons_jsp._jspService basic_002dcomparisons_jsp.java:179 #6 org/apache/jasper/runtime/HttpJspBase.service HttpJspBase.java:70 #7 javax/servlet/http/HttpServlet.service HttpServlet.java:717 #8 org/apache/jasper/servlet/JspServletWrapper.service JspServletWrapper.java:377 #9 org/apache/jasper/servlet/JspServlet.serviceJspFile JspServlet.java:313
(In reply to comment #6) > Lock at this stack-traces to see how this method can be on invoked from a > number of threads: But is each thread in the pair using the same instance? There's no evidence of that in the published trace.
(In reply to comment #7) > (In reply to comment #6) > > Lock at this stack-traces to see how this method can be on invoked from a > > number of threads: > > But is each thread in the pair using the same instance? There's no evidence of > that in the published trace. I omit first line of race report, sorry. WARNING: Possible data race during write of size 1 at 0x76a1569c: {{{ ... Where 0x76a1569c is hashCode of object and field. Also I always add something like this in critical sections: int hash = System.identityHashCode(this); long tid = Thread.currentThread().getId(); System.out.println(">>VER>> START WRITE/READ " + tid + " " + hash); try { Thread.sleep(10); } catch (InterruptedException e) { throw new RuntimeException(e); } System.out.println(">>VER>> END WRITE/READ " + tid + " " + hash); And analise stdout. This race was verified.
Really a pity some attention has been paid to the drones during Tomcat 7 development, I knew it would just attract more of them :)
Fixed in trunk (will be in 7.0.3 onwards) and proposed for 6.0.x. There are many places were static analysis incorrectly reports thread safety issues in the Tomcat code base. Bug reports that look like copy and pasted output from these tools tends to get ignored / resolved as invalid. In this case it took a close inspection of the EL parser to spot the place where these objects were cached to confirm this as a potential issue. If you have done the analysis that demonstrates that there really is an issue it saves a lot of time all round if you include that in the initial bug report.
(In reply to comment #10) > Fixed in trunk (will be in 7.0.3 onwards) and proposed for 6.0.x. > > There are many places were static analysis This is why we use dynamic analysis :) We are experimenting with ThreadSanitizer for Java (search for it if curious). Not something we can offer to users yet, but already 'sort of works for us'. > incorrectly reports thread safety > issues in the Tomcat code base. Bug reports that look like copy and pasted > output from these tools tends to get ignored / resolved as invalid. In this > case it took a close inspection of the EL parser to spot the place where these > objects were cached to confirm this as a potential issue. > > If you have done the analysis that demonstrates that there really is an issue > it saves a lot of time all round if you include that in the initial bug report. Agree.
(In reply to comment #10) > (...) In this > case it took a close inspection of the EL parser to spot the place where these > objects were cached to confirm this as a potential issue. > For reference: the caching of EL nodes is performed by o.a.el.lang.ExpressionBuilder#createNodeInternal(String) (using o.a.el.lang.ExpressionBuilder#cache)
This has been fixed in 6.0.x and will be included in 6.0.30 onwards.