http://svn.apache.org/viewvc/tomcat/taglibs/standard/tags/taglibs-standard-1.2.1/jstlel/src/main/java/org/apache/taglibs/standard/lang/jstl/Evaluator.java?view=markup#l70 Evaluator uses a single static instance of an ELEvaluator. As a result enabling and disabling the cache would apply to all validate and evaluate operations from all threads. This will result in unexpected behaviour if validation is performed on multiple threads concurrently. This is not an issue when the library is included with a web application as multiple copies of the Evaluator will be created for the different ClassLoaders (although that may have other unanticipated consequences). This may be an issue if the library is shared between applications.
This concurrency issue was also hit in EAP/Wildfly. It is an issue even if the library is included inside a web application, e.g. when two different JSPs are requested and evaluated concurrently. Tested with taglibs-standard-jstlel 1.2.5. I'm attaching a suggested patch.
Created attachment 35602 [details] Patch
I believe this patch reduces the window for the race but does not fully close it as another thread could still modify the field between when it is set and then read. It would be unlikely due to variable caching but still possible.
The current implementation disables caching while expressions are being validated during the translation process. This has the side effect of disabling the cache for evaluations that occur concurrently i.e. if a JSP is being translated at the same time a request is occurring. I don't think this was the intent but hesitate to change the behaviour given the age of this library. If we think this is safe, we could simply pass the flag as a parameter to parseExpressionString and eliminate the field entirely.
Sorry for not elaborating before - in fact my main motivation was that currently the parseExpressionString() method can throw a NPE because of the race, which breaks the request processing. Moreover when the NPE occurs, the mBypassCache flag is not reset to false by the calling method Evaluator#validate() and that affects later calls. The patch seemed like a safe solution to prevent the NPE. The caching could still be affected by the race condition, but that seemed like a smaller problem. Passing the flag as a method parameter would be ideal. Perhaps an overloaded method with an extra parameter could be introduced while the old one is kept, to stay backward compatible in case someone is calling the methods directly. I opened a PR in that regard in our fork: https://github.com/jboss/jboss-jstl-api_spec/pull/16/files (I overloaded also convertStaticValueToExpectedType(), which is too using the flag.) I understand the reluctance though.
Linking the JBoss issue for reference: https://issues.jboss.org/browse/JBEE-183
Jeremy, what do you think about the PR? https://github.com/jboss/jboss-jstl-api_spec/pull/16/files If you don't want to change current behaviour, what about at least preventing the NPE from happening (the patch in my first attachment would do that)?
Created attachment 35845 [details] Adds new public method that allows the cache to be bypassed when parsing expressions. Based on Tomas's PR, but rather then change the public method signature this adds a new method that allows the cache to be bypassed. It also tightens up the package-private access and synchronization around access the expression cache.
Patch applied in revision 1828458 https://svn.apache.org/viewvc?view=revision&revision=1828458 @Tomas, do you have a reliable repro that can be used to test it?
Yes, I think I do, but I'm afraid it's gonna involve Wildfly. I will check it out and get back here.
I can no longer reproduce the issue with patched version of jstlel. Thanks Jeremy!
Fix will be included in 1.2.6