Yet another thread might explicitly *remove* the key between the
putIfAbsent and following get; so the above really needs to be done in
a loop.
This is a good point and probably the only real issue of your initial code (and in the latest version of my code, the one without
synchronized blocks); I would suggest the following fix:
Class scriptClass = parsedScripts.get(script);
if (scriptClass == null) {
scriptClass = genClass();
Class cachedScriptClass = null;
do {
parsedScripts.putIfAbsent(script, scriptClass);
cachedScriptClass = parsedScripts.get(script);
} while(cachedScriptClass != null);
scriptClass = cachedScriptClass;
}
We actually don't need the loop here; I have simplified the code as follows:
Class<?> scriptClass = parsedScripts.get(script);
if (scriptClass == null) {
scriptClass = loadClass(script);
Class<?> cachedScriptClass = parsedScripts.putIfAbsent(script,
scriptClass);
if (cachedScriptClass == null) { // putIfAbsent returns null if
the class is added
if (Debug.verboseOn()) {
Debug.logVerbose("Cached Groovy script at: " + script,
module);
}
} else {
scriptClass = cachedScriptClass;
}
}
return InvokerHelper.createScript(scriptClass,
getBinding(context)).run();
I am going to commit this to trunk, 12.04 and 11.04 (will do later today when
the tests will be fixed).
As regards the 10.04, what if we backport the putIfAbsent method there and then apply a similar fix? Here is the patch I would
like to backport:
Index: framework/base/src/org/ofbiz/base/util/cache/test/UtilCacheTests.java
===================================================================
--- framework/base/src/org/ofbiz/base/util/cache/test/UtilCacheTests.java
(revision 1337449)
+++ framework/base/src/org/ofbiz/base/util/cache/test/UtilCacheTests.java
(working copy)
@@ -323,6 +323,19 @@
basicTest(cache);
}
+ public void testPutIfAbsent() throws Exception {
+ UtilCache<String, String> cache = createUtilCache(5, 5, 2000, false,
false);
+ Listener<String, String> gotListener = createListener(cache);
+ Listener<String, String> wantedListener = new Listener<String,
String>();
+ wantedListener.noteKeyAddition(cache, "two", "dos");
+ assertNull("putIfAbsent", cache.putIfAbsent("two", "dos"));
+ assertHasSingleKey(cache, "two", "dos");
+ assertEquals("putIfAbsent", "dos", cache.putIfAbsent("two", "double"));
+ assertHasSingleKey(cache, "two", "dos");
+ cache.removeListener(gotListener);
+ assertEquals("listener", wantedListener, gotListener);
+ }
+
public void testChangeMemSize() throws Exception {
int size = 5;
long ttl = 2000;
Index: framework/base/src/org/ofbiz/base/util/cache/UtilCache.java
===================================================================
--- framework/base/src/org/ofbiz/base/util/cache/UtilCache.java (revision
1337449)
+++ framework/base/src/org/ofbiz/base/util/cache/UtilCache.java (working copy)
@@ -289,6 +289,10 @@
return putInternal(key, value, expireTimeNanos);
}
+ public V putIfAbsent(K key, V value) {
+ return putIfAbsentInternal(key, value, expireTimeNanos);
+ }
+
CacheLine<V> createSoftRefCacheLine(final Object key, V value, long
loadTimeNanos, long expireTimeNanos) {
return tryRegister(loadTimeNanos, new SoftRefCacheLine<V>(value,
loadTimeNanos, expireTimeNanos) {
@Override
@@ -366,6 +370,10 @@
return putInternal(key, value,
TimeUnit.NANOSECONDS.convert(expireTimeMillis, TimeUnit.MILLISECONDS));
}
+ public V putIfAbsent(K key, V value, long expireTimeMillis) {
+ return putIfAbsentInternal(key, value,
TimeUnit.NANOSECONDS.convert(expireTimeMillis, TimeUnit.MILLISECONDS));
+ }
+
V putInternal(K key, V value, long expireTimeNanos) {
Object nulledKey = fromKey(key);
CacheLine<V> oldCacheLine = memoryTable.put(nulledKey,
createCacheLine(key, value, expireTimeNanos));
@@ -388,6 +396,41 @@
}
}
+ V putIfAbsentInternal(K key, V value, long expireTimeNanos) {
+ Object nulledKey = fromKey(key);
+ V oldValue;
+ if (fileTable != null) {
+ try {
+ synchronized (this) {
+ oldValue = fileTable.get(nulledKey);
+ if (oldValue == null) {
+ memoryTable.put(nulledKey, createCacheLine(key, value,
expireTimeNanos));
+ fileTable.put(nulledKey, value);
+ jdbmMgr.commit();
+ }
+ }
+ } catch (IOException e) {
+ Debug.logError(e, module);
+ oldValue = null;
+ }
+ } else {
+ CacheLine<V> newCacheLine = createCacheLine(key, value,
expireTimeNanos);
+ CacheLine<V> oldCacheLine = memoryTable.putIfAbsent(nulledKey,
newCacheLine);
+ if (oldCacheLine == null) {
+ oldValue = null;
+ } else {
+ oldValue = oldCacheLine.getValue();
+ cancel(newCacheLine);
+ }
+ }
+ if (oldValue == null) {
+ noteAddition(key, value);
+ return null;
+ } else {
+ return oldValue;
+ }
+ }
+
/** Gets an element from the cache according to the specified key.
* @param key The key for the element, used to reference it in the
hastables and LRU linked list
* @return The value of the element specified by the key
Index: framework/base/src/org/ofbiz/base/util/GroovyUtil.java
===================================================================
--- framework/base/src/org/ofbiz/base/util/GroovyUtil.java (revision 1337449)
+++ framework/base/src/org/ofbiz/base/util/GroovyUtil.java (working copy)
@@ -102,26 +102,20 @@
public static Class<?> getScriptClassFromLocation(String location) throws
GeneralException {
try {
- Class<?> scriptClass = null;
- synchronized (parsedScripts) {
- scriptClass = parsedScripts.get(location);
- }
+ Class<?> scriptClass = parsedScripts.get(location);
if (scriptClass == null) {
URL scriptUrl = FlexibleLocation.resolveLocation(location);
if (scriptUrl == null) {
throw new GeneralException("Script not found at location [" +
location + "]");
}
scriptClass = parseClass(scriptUrl.openStream(), location);
- 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;
+ Class<?> scriptClassCached =
parsedScripts.putIfAbsent(location, scriptClass);
+ if (scriptClassCached == null) { // putIfAbsent returns null
if the class is added
+ if (Debug.verboseOn()) {
+ Debug.logVerbose("Cached Groovy script at: " +
location, module);
}
+ } else {
+ scriptClass = scriptClassCached;
}
}
return scriptClass;
@@ -152,22 +146,16 @@
public static Object runScriptFromClasspath(String script,
Map<String,Object> context) throws GeneralException {
try {
- Class<?> scriptClass = null;
- synchronized (parsedScripts) {
- parsedScripts.get(script);
- }
+ Class<?> scriptClass = parsedScripts.get(script);
if (scriptClass == null) {
scriptClass = loadClass(script);
- 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;
+ Class<?> cachedScriptClass = parsedScripts.putIfAbsent(script,
scriptClass);
+ if (cachedScriptClass == null) { // putIfAbsent returns null
if the class is added
+ if (Debug.verboseOn()) {
+ Debug.logVerbose("Cached Groovy script at: " + script,
module);
}
+ } else {
+ scriptClass = cachedScriptClass;
}
}
return InvokerHelper.createScript(scriptClass,
getBinding(context)).run();
