On 04/20/2012 07:53 AM, [email protected] wrote:
Author: jacopoc
Date: Fri Apr 20 12:53:35 2012
New Revision: 1328356
URL: http://svn.apache.org/viewvc?rev=1328356&view=rev
Log:
The cache of parsed Groovy scripts was not thread safe; this issue, in
instances with several concurrent threads running the same script the first
time (i.e. not cached) could cause the same script parsed multiple times and
then added to the cache (overriding the previous value); this event was causing
the clearing of caches in Freemarker; because of a bug in Freemarker [*] this
could cause a deadlock.
The issue is present on all versions of Freemarker but it is less frequent on
latest version because of the refactoring of caches happened after release
2.3.10.
[*]
https://sourceforge.net/tracker/?func=detail&aid=3519805&group_id=794&atid=100794
Modified:
ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Modified: ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
URL:
http://svn.apache.org/viewvc/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java?rev=1328356&r1=1328355&r2=1328356&view=diff
==============================================================================
--- ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
(original)
+++ ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java Fri Apr
20 12:53:35 2012
@@ -127,10 +127,17 @@ public class GroovyUtil {
} else {
scriptClass = parseClass(scriptUrl.openStream(),
location);
}
- if (Debug.verboseOn()) {
- Debug.logVerbose("Caching Groovy script at: " + location,
module);
+ synchronized (parsedScripts) {
+ Class<?> scriptClassCached = parsedScripts.get(location);
+ if (scriptClassCached == null) {
+ if (Debug.verboseOn()) {
+ Debug.logVerbose("Caching Groovy script at: " +
location, module);
+ }
+ parsedScripts.put(location, scriptClass);
+ } else {
+ scriptClass = scriptClassCached;
+ }
}
- parsedScripts.put(location, scriptClass);
}
return scriptClass;
} catch (Exception e) {
@@ -177,10 +184,18 @@ public class GroovyUtil {
Class<?> scriptClass = parsedScripts.get(script);
if (scriptClass == null) {
scriptClass = loadClass(script);
- if (Debug.verboseOn()) Debug.logVerbose("Caching Groovy script:
" + script, module);
- parsedScripts.put(script, scriptClass);
+ synchronized (parsedScripts) {
+ Class<?> scriptClassCached = parsedScripts.get(script);
+ if (scriptClassCached == null) {
+ if (Debug.verboseOn()) {
+ Debug.logVerbose("Caching Groovy script: " +
script, module);
+ }
+ parsedScripts.put(script, scriptClass);
+ } else {
+ scriptClass = scriptClassCached;
+ }
+ }
}
-
return InvokerHelper.createScript(scriptClass,
getBinding(context)).run();
} catch (CompilationFailedException e) {
String errMsg = "Error loading Groovy script [" + script + "]: "
+ e.toString();
Er, no, this is the *wrong* way to fix this. parsedScripts is a
UtilCache instance, not a Map, not even a HashMap. It is *designed* to
not corrupt without synchronization.
The proper fix is:
Class scriptClass = parsedScripts.get(script);
if (scriptClass == null) {
scriptClass = genClass();
parsedScripts.putIfAbsent(script, scriptClass);
scriptClass = parsedScripts.get(script);
}
What was the real problem you were attempting to fix? The only issue
that would have happened with the previous code was the case of a static
variable being defined in the parsed script, and during a race condition
while parsing, 2 separate classes for the same script being in the system.
This change is in the release branch. I'd like to know the real reason
this was changed in ofbiz, and then my fix applied to the branch.
ps: I can't recommend it enough, but in addition to the entity model
data resource books, everyone working on ofbiz should also be reading
java concurrency in practice.
pps: It is my goal to remove all the DCL in ofbiz; I'd love it if others
would help.
