Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java

[Adam Heath-2]

On 05/11/2012 02:46 PM, Jacopo Cappellato wrote:
>> The UtilCache objects *are* thread-safe.  You will not get any
>> internal corruption inside UtilCache.  That is all that can be guaranteed.
>>
>> What isn't thread safe, is that calling code may end up having
>> multiple instances for the same key, unless calling code is written
>> correctly.
>
> And so if these methods are thread safe all your comments below don't apply to the code I wrote:

but if 2 threads both want foo, *and* there must only be *one*
singleton foo value, without an external synchronization point, there
might be 2 foo values in the system.  This can't be fixed with
anything internal to the storage system.

(I've wanted to add a UtilCache.getOrCreate(key, creator) method for a
while to solve this problem, but have never gotten around to it).

>>> Saying again.  2 threads could be running.  One wants "foo", the other
>>> wants "bar".  The first sees no value for "foo", starts generating the
>>> result, then calls map.put("foo", result).  When map.put is running,
>>> it's doing a lot of separate, individual steps.
>>>
>>> Now, the second thread comes around, and calls map.get("bar").  The
>>> first thread is still running inside the put.  The second thread is
>>> not in the synchronized block, so it went straight into map.get.  And
>>> now you have 2 threads both running code inside that map that has no
>>> happens-before/happens-after guarantees, so now things blow up.

[Jacopo Cappellato-4]

On May 11, 2012, at 9:51 PM, Adam Heath wrote:

> On 05/11/2012 02:46 PM, Jacopo Cappellato wrote:
>>> The UtilCache objects *are* thread-safe.  You will not get any
>>> internal corruption inside UtilCache.  That is all that can be guaranteed.
>>>
>>> What isn't thread safe, is that calling code may end up having
>>> multiple instances for the same key, unless calling code is written
>>> correctly.
>>
>> And so if these methods are thread safe all your comments below don't apply to the code I wrote:
>
> but if 2 threads both want foo, *and* there must only be *one*
> singleton foo value, without an external synchronization point, there
> might be 2 foo values in the system.  This can't be fixed with
> anything internal to the storage system.
>

Yes, of course if you rely on this mechanism to figure out if you need to run a critical initialization that has to run only *one* time (a singleton, some special file creation, db operations, communication with external systems etc...) you definitely need an external mechanism of synchronization.
But this is not the case here and for this reason your comments don't apply to the code I wrote (there is no harm at having two instances of the same parsed script here, it is just a small waste of cpu that I was aware about and it is acceptable).

Jacopo

[Jacopo Cappellato-4]

On May 11, 2012, at 9:51 PM, Adam Heath wrote:

> (I've wanted to add a UtilCache.getOrCreate(key, creator) method for a
> while to solve this problem, but have never gotten around to it).

Nice.
The following small utility method may also be useful:

Index: framework/base/src/org/ofbiz/base/util/cache/UtilCache.java
===================================================================
--- framework/base/src/org/ofbiz/base/util/cache/UtilCache.java (revision 1337191)
+++ framework/base/src/org/ofbiz/base/util/cache/UtilCache.java (working copy)
@@ -289,6 +289,11 @@
         return putIfAbsentInternal(key, value, expireTimeNanos);
     }
 
+    public V putIfAbsentAndGet(K key, V value) {
+        V cachedValue = putIfAbsent(key, value);
+        return (cachedValue != null? cachedValue: value);
+    }
+
     CacheLine<V> createSoftRefCacheLine(final Object key, V value, long loadTimeNanos, long expireTimeNanos) {
         return tryRegister(loadTimeNanos, new SoftRefCacheLine<V>(value, loadTimeNanos, expireTimeNanos) {
             @Override

Jacopo

[Jacopo Cappellato-4]

>> 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();

[Jacques Le Roux]

I'm sure you are both afraid of that, but in case you freithened people about DCL I wanted to say that since jdk 1.5 way of handling
volatile it's now possible to safely use this pattern with Java
http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html#ThreadLocal
(see "Fixing Double-Checked Locking using Volatile" section)

Actually it was not my main point. Some months ago I read 2 other very simple and interesting ways (not much code, and simple) to
handle the same goal, but I can't find the article anymore :/

Jacques

From: "Jacopo Cappellato" <[hidden email]>

>>> 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();
>
>

[Adam Heath-2]

On 05/12/2012 04:28 PM, Jacques Le Roux wrote:

> I'm sure you are both afraid of that, but in case you freithened people
> about DCL I wanted to say that since jdk 1.5 way of handling volatile
> it's now possible to safely use this pattern with Java
> http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html#ThreadLocal
>
> (see "Fixing Double-Checked Locking using Volatile" section)
>
> Actually it was not my main point. Some months ago I read 2 other very
> simple and interesting ways (not much code, and simple) to handle the
> same goal, but I can't find the article anymore :/

Sure, volatile makes DCL *safe*, but it is still slower, in the case
when the initial fetch is null.  For static initialization, it might be
ok to use it.  But it's probably more confusing to unaware java
programmers(using volatile+DCL).  Using CAS-type mechanisms makes it
more explicit what is actually occuring.

And, when DCL is used against map-type(or collection-type) systems,
where the value could go away at any time, removing DCL can actually
make things faster, as you are reducing contention.

[Jacques Le Roux]

From: "Adam Heath" <[hidden email]>
> On 05/12/2012 04:28 PM, Jacques Le Roux wrote:
>> I'm sure you are both afraid of that, but in case you freithened people

Oops, was late it seems. I meant " I'm sure you are both *aware" of that :o)

>> about DCL I wanted to say that since jdk 1.5 way of handling volatile
>> it's now possible to safely use this pattern with Java
>> http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html#ThreadLocal
>>
>> (see "Fixing Double-Checked Locking using Volatile" section)
>>
>> Actually it was not my main point. Some months ago I read 2 other very
>> simple and interesting ways (not much code, and simple) to handle the
>> same goal, but I can't find the article anymore :/
>
> Sure, volatile makes DCL *safe*, but it is still slower, in the case
> when the initial fetch is null.  For static initialization, it might be
> ok to use it.  But it's probably more confusing to unaware java
> programmers(using volatile+DCL).  Using CAS-type mechanisms makes it
> more explicit what is actually occuring.
>
> And, when DCL is used against map-type(or collection-type) systems,
> where the value could go away at any time, removing DCL can actually
> make things faster, as you are reducing contention.

Yes using volatile with DCL is defintitvely slower. What do you call CAS-type?

Jacques

[Adam Heath-2]

On 05/13/2012 02:24 AM, Jacques Le Roux wrote:

> From: "Adam Heath" <[hidden email]>
>> Sure, volatile makes DCL *safe*, but it is still slower, in the case
>> when the initial fetch is null.  For static initialization, it might
>> be ok to use it.  But it's probably more confusing to unaware java
>> programmers(using volatile+DCL).  Using CAS-type mechanisms makes it
>> more explicit what is actually occuring.
>>
>> And, when DCL is used against map-type(or collection-type) systems,
>> where the value could go away at any time, removing DCL can actually
>> make things faster, as you are reducing contention.
>
> Yes using volatile with DCL is defintitvely slower. What do you call
> CAS-type?

java.util.concurrent.atomic.* for instance.  The sun jdk jit compiler
actually detects calls to those objects, and replaces them with
low-level asm calls instead.  No object overhead.  volatile
effectively means no cpu-caching on die.
> Jacques

[Jacopo Cappellato-4]

Adam, all,

I am resurrecting this part of the thread: is it ok if we fix the 10.04 with the patch below (Adam's commit with rev. 948333 that adds the putIfAbsent method and then the same fix I committed to 11.04, 12.04 and trunk)?

Thanks,

Jacopo

On May 12, 2012, at 9:17 AM, Jacopo Cappellato wrote:

> 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();