You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by ja...@apache.org on 2012/04/20 14:53:35 UTC
svn commit: r1328356 -
/ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
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();
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Adrian Crum <ad...@sandglass-software.com>.
On 5/11/2012 3:52 AM, Adam Heath wrote:
> On 04/20/2012 07:53 AM, jacopoc@apache.org 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.
I remove DCL when I come across it, but I haven't turned it into a task.
My recent work on Mini-language fixed some concurrency issues.
-Adrian
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On May 11, 2012, at 4:52 AM, Adam Heath wrote:
> 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.
I have backported a new version of my fix to 12.04 (in rev. 1337054) and 11.04 (in rev. 1337097); I didn't backport to 10.04 because putIfAbsent was implemented after the branch was created and it is not available there (but the old version of my fix is there and will be fine).
Jacopo
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On May 11, 2012, at 3:53 PM, Adam Heath wrote:
>
> The code above that I originally quoted is doing DCL.
You are wrong; the DCL (anti) pattern is:
if (sharedResource == null) {
synchronized(lock) {
sharedResource = new Resource();
}
}
what I did is completely different because scriptClass is a *local* variable; please read carefully.
Jacopo
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On May 11, 2012, at 3:53 PM, Adam Heath wrote:
> Haven't yet looked at those urls, but just based on your description, the only way to fix code that has broken locking is to wrap it in synchronized. If you use non-locking techniques, the code being called might still be run in parallel, which won't help the situation.
>
> Let me look into this further before you this code is changed.
>
> If I find out that there is a fixed freemarker(or, I come up with a patch), then I assume we'd be for including that fixed freemarker. What about backporting that to other branches(just the locking fix for freemarker, plus whatever locking fix in ofbiz)?
I already proposed a fix for the deadlock condition in freemarker, they asked me to send a signed icla (I already sent it to them) and we will have to wait a bit before it is committed.
The fix I did in OFBiz fixes exactly what I described:
* before my fix the same script could be parsed multiple times and added to the cache by several threads running a script not in the cache: at the end the parsed script in the cache was the one processed by the slowest thread (i.e. each thread was overriding the value put by the previous thread)
* after my fix the same script could be parsed multiple times by several threads running a script not in the cache; but only the fastest thread is now adding a value to the cache: at the end the parsed script in the cache is the one processed by the fastest thread
Please read the whole story if you are interested but at this point the problem (yes, the problem was real) is resolved.
Jacopo
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On May 11, 2012, at 5:49 PM, Jacopo Cappellato wrote:
> is the real issue there (i.e. the DCL anti pattern) because another thread could acquire a lock on helper and initialize it;
oops; errata corrige:
is the real issue there (i.e. the DCL anti pattern) because another thread could acquire a lock on this and initialize helper;
Jacopo
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
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
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
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
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Adam Heath <do...@brainfood.com>.
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.
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On May 11, 2012, at 8:00 PM, Adam Heath wrote:
> On 05/11/2012 12:30 PM, Jacopo Cappellato wrote:
>>
>> On May 11, 2012, at 6:34 PM, Adam Heath wrote:
>>
>>> There is no difference. The act of map.get() can be doing a *lot* of
>>> separate, individual steps. Without any kind of
>>> happens-before/happens-after on map.get(), the protected put/get in
>>> the lock could interleave with the unprotected get.
>>
>> Ok, if you don't want to admit that there is no DCL in my code I will give up; what you describe next is a clearly totally different topic.
>
> I won't admit that, because there IS a DCL in the code you added in
> the original commit.
>
> if (!initialized) {
> lock {
> if (!initialized) {
> initialize();
> }
> }
> }
>
This is not what I implemented; I already explained it in my other emails, please read them carefully.I have to go now but if you can't find the answers from my others email let me know and I will add details during the week end (now I have to go).
> That is DCL to. It doesn't matter if initialized is a simple read of
> a variable, a null check, or something more complex(map.get). They
> are all the same *pattern*.
>
> What is at issue, is that the code running before the lock can be
> interleaved with the code run inside the lock. This is because the
> code outside the lock has no barrier, no guard, no kind of
> happens-relationship at all with the code inside the lock. All the
> lock helps protect is 2 threads trying to run the internal block. But
> that external block can run at any time.
There is no need to explain to me why DCL is bad.
>
>> But even if a totally different topic I think it is an interesting one because the UtilCache get/put/putIfAbsent methods are used a lot and if they are not thread safe this would be a problem. A problem for my code, your code and a lot of code in OFBiz.
>
> 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:
>> 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.
>
> The changes I did to remove synchronized from most places in UtilCache
> were correct, it reduced contention. Each method on UtilCache still
> has correct happens-before/happens-since, both before and after
> removing synchronized.
>
And in fact I greatly appreciate the work you did at that time.
> Any such issues in calling code were always there previously; it's
> just that because of synchronized, it slowed down threads enough so
> that you wouldn't see bad code as often. Remove the contention,
> things happen quicker, and so do any bugs.
>
> Why do you think java 1.1 introduced HashMap, to replace Hashtable?
> Hashtable used to have synchronized on get/put, but in reality, that
> did *nothing* to protect calling code that had *separate* calls to
> get/put/containsKey/whatever. It is the *external* code that needed
> the lock, not the Hashtable.
>
> Also, an unprotected map.entrySet().iterator() can have corruption if
> other threads are mutating the structure of the map. In those case,
> external synchronization needs to happen between the iteration, *and*
> the get/put/remove. If you read the ConcurrentMap(and friends) code,
> you'll find references to the iterators failing
> fast(ConcurrentModificationException), or not failing at all(you'll
> get a read-only snapshot of something that may not reflect the current
> values of the Collection).
Yeah, I know all of these things: "Java Concurrency In Practice" is actually quite enlightening on these topics.
Regards,
Jacopo
>
>> I didn't look too closely at the implementation of the two methods but it seems that they are actually handling the shared status in a thread safe way (even if they are not atomic), so I do not see a risk for reading objects in a stale/invalid status; but you actually did, if I am not wrong, the implementation of these method and you may know better than I.
>
> I have seen a map.get/null-check/put DCL as given in your code cause
> threads to enter into never-ending loops, as they call .toString(),
> which calls .iterator(), which, because of unprotected manipulation,
> the hash-bucket links internal to the map become circular.
>
> The first time(it's happened multiple times), I had no clue. But
> then, I added a -XX parameter to allow me to attach to the java
> process. The next time, I dumped the entire memory(that was huge, as
> these bugs happen more often during OOM). I then used IBM's
> HeapAnaylzer(nice little swing app to memory dumps, better than jhat)
> to see that the hashbucket links were circular.
>
>> By the way, all the issues that you are mentioning are actually all present in the fix that you suggested at the beginning of this thread:
>>
>> Class scriptClass = parsedScripts.get(script);
>> if (scriptClass == null) {
>> scriptClass = genClass();
>> parsedScripts.putIfAbsent(script, scriptClass);
>> scriptClass = parsedScripts.get(script);
>> }
>
> parsedScripts is a UtilCache; the methods on that object have
> happens-before/happens-after guarantees, due to internally using
> ConcurrentMap. Deep inside ConcurrentMap, you'll find variables
> marked volatile, and/or work-stealing/work-completing patterns, that
> also again have happens-before/happens-after.
>
> The only issue in the above code, as I have already mentioned, is that
> some other background thread may call remove(script) inbetween
> putIfAbsent() and the following get(). And the solution for that is
> to change the if to a while.
>
>> while my initial code was safer (because it was wrapping get and put calls into synchronized blocks). But if the UtilCache method put/get/putIfAbsent are not thread safe, now it makes me nervous to call them within synchronized blocks because in them there are also synchronized blocks (risk for deadlocks). The best way to fix this would be to make the methods thread safe (if they are not already as it seems they are).
>
> Nope, your code still had the same removal problem.
>
>>> Yet another thread might explicitly *remove* the key between the
>>> putIfAsent and following get; so the above really needs to be done in
>>> a loop. With UtilCache, an item may be removed *at any time* when
>>> soft-references are used, or when a TTL expires. Even in such a short
>>> timeframe as the 2 lines above. Just because those 2 lines are right
>>> next to each other, doesn't mean that the process execution context
>>> will stay running this particular thread.
>>
>> 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;
>> }
>>
>> But of course here I am assuming that UtilCache put/get/putIfAbsent are thread safe.
>
> They are, just without doing synchronized.
>
>> What do you think?
>>
>> Jacopo
>>
>
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
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();
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Adam Heath <do...@brainfood.com>.
On 05/13/2012 02:24 AM, Jacques Le Roux wrote:
> From: "Adam Heath" <do...@brainfood.com>
>> 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
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
From: "Adam Heath" <do...@brainfood.com>
> 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
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Adam Heath <do...@brainfood.com>.
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.
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacques Le Roux <ja...@les7arts.com>.
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" <ja...@hotwaxmedia.com>
>>> 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();
>
>
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
>> 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();
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Adam Heath <do...@brainfood.com>.
On 05/11/2012 12:30 PM, Jacopo Cappellato wrote:
>
> On May 11, 2012, at 6:34 PM, Adam Heath wrote:
>
>> There is no difference. The act of map.get() can be doing a *lot* of
>> separate, individual steps. Without any kind of
>> happens-before/happens-after on map.get(), the protected put/get in
>> the lock could interleave with the unprotected get.
>
> Ok, if you don't want to admit that there is no DCL in my code I will give up; what you describe next is a clearly totally different topic.
I won't admit that, because there IS a DCL in the code you added in
the original commit.
if (!initialized) {
lock {
if (!initialized) {
initialize();
}
}
}
That is DCL to. It doesn't matter if initialized is a simple read of
a variable, a null check, or something more complex(map.get). They
are all the same *pattern*.
What is at issue, is that the code running before the lock can be
interleaved with the code run inside the lock. This is because the
code outside the lock has no barrier, no guard, no kind of
happens-relationship at all with the code inside the lock. All the
lock helps protect is 2 threads trying to run the internal block. But
that external block can run at any time.
> But even if a totally different topic I think it is an interesting one because the UtilCache get/put/putIfAbsent methods are used a lot and if they are not thread safe this would be a problem. A problem for my code, your code and a lot of code in OFBiz.
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.
The changes I did to remove synchronized from most places in UtilCache
were correct, it reduced contention. Each method on UtilCache still
has correct happens-before/happens-since, both before and after
removing synchronized.
Any such issues in calling code were always there previously; it's
just that because of synchronized, it slowed down threads enough so
that you wouldn't see bad code as often. Remove the contention,
things happen quicker, and so do any bugs.
Why do you think java 1.1 introduced HashMap, to replace Hashtable?
Hashtable used to have synchronized on get/put, but in reality, that
did *nothing* to protect calling code that had *separate* calls to
get/put/containsKey/whatever. It is the *external* code that needed
the lock, not the Hashtable.
Also, an unprotected map.entrySet().iterator() can have corruption if
other threads are mutating the structure of the map. In those case,
external synchronization needs to happen between the iteration, *and*
the get/put/remove. If you read the ConcurrentMap(and friends) code,
you'll find references to the iterators failing
fast(ConcurrentModificationException), or not failing at all(you'll
get a read-only snapshot of something that may not reflect the current
values of the Collection).
> I didn't look too closely at the implementation of the two methods but it seems that they are actually handling the shared status in a thread safe way (even if they are not atomic), so I do not see a risk for reading objects in a stale/invalid status; but you actually did, if I am not wrong, the implementation of these method and you may know better than I.
I have seen a map.get/null-check/put DCL as given in your code cause
threads to enter into never-ending loops, as they call .toString(),
which calls .iterator(), which, because of unprotected manipulation,
the hash-bucket links internal to the map become circular.
The first time(it's happened multiple times), I had no clue. But
then, I added a -XX parameter to allow me to attach to the java
process. The next time, I dumped the entire memory(that was huge, as
these bugs happen more often during OOM). I then used IBM's
HeapAnaylzer(nice little swing app to memory dumps, better than jhat)
to see that the hashbucket links were circular.
> By the way, all the issues that you are mentioning are actually all present in the fix that you suggested at the beginning of this thread:
>
> Class scriptClass = parsedScripts.get(script);
> if (scriptClass == null) {
> scriptClass = genClass();
> parsedScripts.putIfAbsent(script, scriptClass);
> scriptClass = parsedScripts.get(script);
> }
parsedScripts is a UtilCache; the methods on that object have
happens-before/happens-after guarantees, due to internally using
ConcurrentMap. Deep inside ConcurrentMap, you'll find variables
marked volatile, and/or work-stealing/work-completing patterns, that
also again have happens-before/happens-after.
The only issue in the above code, as I have already mentioned, is that
some other background thread may call remove(script) inbetween
putIfAbsent() and the following get(). And the solution for that is
to change the if to a while.
> while my initial code was safer (because it was wrapping get and put calls into synchronized blocks). But if the UtilCache method put/get/putIfAbsent are not thread safe, now it makes me nervous to call them within synchronized blocks because in them there are also synchronized blocks (risk for deadlocks). The best way to fix this would be to make the methods thread safe (if they are not already as it seems they are).
Nope, your code still had the same removal problem.
>> Yet another thread might explicitly *remove* the key between the
>> putIfAsent and following get; so the above really needs to be done in
>> a loop. With UtilCache, an item may be removed *at any time* when
>> soft-references are used, or when a TTL expires. Even in such a short
>> timeframe as the 2 lines above. Just because those 2 lines are right
>> next to each other, doesn't mean that the process execution context
>> will stay running this particular thread.
>
> 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;
> }
>
> But of course here I am assuming that UtilCache put/get/putIfAbsent are thread safe.
They are, just without doing synchronized.
> What do you think?
>
> Jacopo
>
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On May 11, 2012, at 6:34 PM, Adam Heath wrote:
> There is no difference. The act of map.get() can be doing a *lot* of
> separate, individual steps. Without any kind of
> happens-before/happens-after on map.get(), the protected put/get in
> the lock could interleave with the unprotected get.
Ok, if you don't want to admit that there is no DCL in my code I will give up; what you describe next is a clearly totally different topic.
But even if a totally different topic I think it is an interesting one because the UtilCache get/put/putIfAbsent methods are used a lot and if they are not thread safe this would be a problem. A problem for my code, your code and a lot of code in OFBiz.
I didn't look too closely at the implementation of the two methods but it seems that they are actually handling the shared status in a thread safe way (even if they are not atomic), so I do not see a risk for reading objects in a stale/invalid status; but you actually did, if I am not wrong, the implementation of these method and you may know better than I.
By the way, all the issues that you are mentioning are actually all present in the fix that you suggested at the beginning of this thread:
Class scriptClass = parsedScripts.get(script);
if (scriptClass == null) {
scriptClass = genClass();
parsedScripts.putIfAbsent(script, scriptClass);
scriptClass = parsedScripts.get(script);
}
while my initial code was safer (because it was wrapping get and put calls into synchronized blocks). But if the UtilCache method put/get/putIfAbsent are not thread safe, now it makes me nervous to call them within synchronized blocks because in them there are also synchronized blocks (risk for deadlocks). The best way to fix this would be to make the methods thread safe (if they are not already as it seems they are).
> Yet another thread might explicitly *remove* the key between the
> putIfAsent and following get; so the above really needs to be done in
> a loop. With UtilCache, an item may be removed *at any time* when
> soft-references are used, or when a TTL expires. Even in such a short
> timeframe as the 2 lines above. Just because those 2 lines are right
> next to each other, doesn't mean that the process execution context
> will stay running this particular thread.
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;
}
But of course here I am assuming that UtilCache put/get/putIfAbsent are thread safe.
What do you think?
Jacopo
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On May 11, 2012, at 6:34 PM, Adam Heath wrote:
>> Inside the synchronized block of your example I see the equivalent of a putIfAbsent method; so your code can be simplified in the equivalent version:
>
> There is no putIfAbsent inside synchronized of any example I have given.
What I meant is that the whole synchronized block is equivalent to a putIfAbsent.
Jacopo
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Adam Heath <do...@brainfood.com>.
On 05/11/2012 10:49 AM, Jacopo Cappellato wrote:
>
> On May 11, 2012, at 5:38 PM, Adam Heath wrote:
>
>> On 05/11/2012 10:30 AM, Jacopo Cappellato wrote:
>>> On May 11, 2012, at 4:51 PM, Adam Heath wrote:
>>>
>>>> ==
>>>> result = parsedScripts.get(key)
>>>> if (result == null) {
>>>> syncyronized (parsedScripts) {
>>>> result = parsedScripts.get(key)
>>>> if (result == null) {
>>>> result = genResult()
>>>> parsedScripts.put(key, result)
>>>> }
>>>> }
>>>> }
>>>> return result
>>>> ==
>>>>
>>>> DCL is about the protected resource, parsedScripts. Please try again.
>>>>
>>>> The clue here it to look at what is being protected by the
>>>> synchronized keyword.
>>>
>>> Just to be sure I understand: are you saying that the above code is an example of the DCL anti-pattern and it is wrong?
>>
>> I'm not saying that. Others are saying that, and I'm just repeating it.
>>
>> http://en.wikipedia.org/wiki/Double-checked_locking
>>
>> ==
>> class Foo {
>> private Helper helper = null;
>> public Helper getHelper() {
>> if (helper == null) {
>> synchronized(this) {
>> if (helper == null) {
>> helper = new Helper();
>> }
>> }
>> }
>> return helper;
>> }
>>
>> // other functions and members...
>> }
>> ==
>>
>> In my example, the protected variable is parsedScripts(instead of
>> this), the get/put on the map is setting the variable on the class.
>
> Adam, don't you really see the big difference between the code from Wikipedia and your example?
> The unprotected line:
There is no difference. The act of map.get() can be doing a *lot* of
separate, individual steps. Without any kind of
happens-before/happens-after on map.get(), the protected put/get in
the lock could interleave with the unprotected get.
You have to remember that map.put/map.get are *not* single
instructions that can't be threaded. When you call map.put inside the
lock, just because *that* thread is protected from other threads that
got a null return on the outer map.get(), doesn't mean that *other*
threads who are requested *other* keys won't attempt to all map.get.
In those situations, you can have internal map memory corruption.
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.
>> if (helper == null) {
>
>
> is the real issue there (i.e. the DCL anti pattern) because another thread could acquire a lock on helper and initialize it; the thread executing the line if (helper == null) { would see a stale/incomplete value at that point.
> On the other hand your example (that is basically what I did in my original code) is not doing an unprotected if (parsedScripts == null) check; it is instead checking a local variable if (result == null)
You can't get a lock on an unassigned variable. So th
>
> Inside the synchronized block of your example I see the equivalent of a putIfAbsent method; so your code can be simplified in the equivalent version:
There is no putIfAbsent inside synchronized of any example I have given.
> result = parsedScripts.get(key);
> if (result == null) {
> result = genResult();
> parsedScripts.putIfAbsent(key, result);
> result = parsedScripts.get(key);
> }
> return result;
And, technically, this is wrong with regard to UtilCache, and possibly
even normal ConcurrentMap.
Yet another thread might explicitly *remove* the key between the
putIfAsent and following get; so the above really needs to be done in
a loop. With UtilCache, an item may be removed *at any time* when
soft-references are used, or when a TTL expires. Even in such a short
timeframe as the 2 lines above. Just because those 2 lines are right
next to each other, doesn't mean that the process execution context
will stay running this particular thread.
> Do we agree that this code is equivalent (well, it is slightly less efficient but it is equivalent as regards the concurrency issue we are discussing)?
> There is nothing wrong in this code and if you compare it with the fix you suggested in your firs email you will see that they are exactly the same.
It's not less efficient. The act of entering the monitored section,
which keeps just *one* thread from ever generating a result, to the
non-locked version, which allows multiple threads to generate results,
is *more* efficient. Especially when you consider that multiple,
completely separate keys may get requested, that don't overlap at all.
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On May 11, 2012, at 5:38 PM, Adam Heath wrote:
> On 05/11/2012 10:30 AM, Jacopo Cappellato wrote:
>> On May 11, 2012, at 4:51 PM, Adam Heath wrote:
>>
>>> ==
>>> result = parsedScripts.get(key)
>>> if (result == null) {
>>> syncyronized (parsedScripts) {
>>> result = parsedScripts.get(key)
>>> if (result == null) {
>>> result = genResult()
>>> parsedScripts.put(key, result)
>>> }
>>> }
>>> }
>>> return result
>>> ==
>>>
>>> DCL is about the protected resource, parsedScripts. Please try again.
>>>
>>> The clue here it to look at what is being protected by the
>>> synchronized keyword.
>>
>> Just to be sure I understand: are you saying that the above code is an example of the DCL anti-pattern and it is wrong?
>
> I'm not saying that. Others are saying that, and I'm just repeating it.
>
> http://en.wikipedia.org/wiki/Double-checked_locking
>
> ==
> class Foo {
> private Helper helper = null;
> public Helper getHelper() {
> if (helper == null) {
> synchronized(this) {
> if (helper == null) {
> helper = new Helper();
> }
> }
> }
> return helper;
> }
>
> // other functions and members...
> }
> ==
>
> In my example, the protected variable is parsedScripts(instead of
> this), the get/put on the map is setting the variable on the class.
Adam, don't you really see the big difference between the code from Wikipedia and your example?
The unprotected line:
> if (helper == null) {
is the real issue there (i.e. the DCL anti pattern) because another thread could acquire a lock on helper and initialize it; the thread executing the line if (helper == null) { would see a stale/incomplete value at that point.
On the other hand your example (that is basically what I did in my original code) is not doing an unprotected if (parsedScripts == null) check; it is instead checking a local variable if (result == null)
Inside the synchronized block of your example I see the equivalent of a putIfAbsent method; so your code can be simplified in the equivalent version:
result = parsedScripts.get(key);
if (result == null) {
result = genResult();
parsedScripts.putIfAbsent(key, result);
result = parsedScripts.get(key);
}
return result;
Do we agree that this code is equivalent (well, it is slightly less efficient but it is equivalent as regards the concurrency issue we are discussing)?
There is nothing wrong in this code and if you compare it with the fix you suggested in your firs email you will see that they are exactly the same.
Jacopo
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Adam Heath <do...@brainfood.com>.
On 05/11/2012 10:30 AM, Jacopo Cappellato wrote:
> On May 11, 2012, at 4:51 PM, Adam Heath wrote:
>
>> ==
>> result = parsedScripts.get(key)
>> if (result == null) {
>> syncyronized (parsedScripts) {
>> result = parsedScripts.get(key)
>> if (result == null) {
>> result = genResult()
>> parsedScripts.put(key, result)
>> }
>> }
>> }
>> return result
>> ==
>>
>> DCL is about the protected resource, parsedScripts. Please try again.
>>
>> The clue here it to look at what is being protected by the
>> synchronized keyword.
>
> Just to be sure I understand: are you saying that the above code is an example of the DCL anti-pattern and it is wrong?
I'm not saying that. Others are saying that, and I'm just repeating it.
http://en.wikipedia.org/wiki/Double-checked_locking
==
class Foo {
private Helper helper = null;
public Helper getHelper() {
if (helper == null) {
synchronized(this) {
if (helper == null) {
helper = new Helper();
}
}
}
return helper;
}
// other functions and members...
}
==
In my example, the protected variable is parsedScripts(instead of
this), the get/put on the map is setting the variable on the class.
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On May 11, 2012, at 4:51 PM, Adam Heath wrote:
> ==
> result = parsedScripts.get(key)
> if (result == null) {
> syncyronized (parsedScripts) {
> result = parsedScripts.get(key)
> if (result == null) {
> result = genResult()
> parsedScripts.put(key, result)
> }
> }
> }
> return result
> ==
>
> DCL is about the protected resource, parsedScripts. Please try again.
>
> The clue here it to look at what is being protected by the
> synchronized keyword.
Just to be sure I understand: are you saying that the above code is an example of the DCL anti-pattern and it is wrong?
Jacopo
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Adam Heath <do...@brainfood.com>.
On 05/11/2012 09:51 AM, Adam Heath wrote:
> On 05/11/2012 09:36 AM, Jacopo Cappellato wrote:
>>
>> On May 11, 2012, at 4:26 PM, Adam Heath wrote:
>>
>>>>>>> @@ -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;
>>>>>>> + }
>>>>>>> + }
>>>>>>> }
>>>>>>> -
>>>
>>> This is DCL.
>>
>> Oh no, you are wrong Adam: it would be DCL if scriptClass was a shared variable, which is not: it is a *local* variable.
And of course scriptClass needs to locking, because of *course* it is
local. But parsedScripts is shared, needs protection, but the pattern
you introduced is the DCL. DCL is about protecting shared resources.
> ==
> result = parsedScripts.get(key)
> if (result == null) {
> syncyronized (parsedScripts) {
> result = parsedScripts.get(key)
> if (result == null) {
> result = genResult()
> parsedScripts.put(key, result)
> }
> }
> }
> return result
> ==
>
> DCL is about the protected resource, parsedScripts. Please try again.
>
> The clue here it to look at what is being protected by the
> synchronized keyword.
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Adam Heath <do...@brainfood.com>.
On 05/11/2012 09:36 AM, Jacopo Cappellato wrote:
>
> On May 11, 2012, at 4:26 PM, Adam Heath wrote:
>
>>>>>> @@ -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;
>>>>>> + }
>>>>>> + }
>>>>>> }
>>>>>> -
>>
>> This is DCL.
>
> Oh no, you are wrong Adam: it would be DCL if scriptClass was a shared variable, which is not: it is a *local* variable.
==
result = parsedScripts.get(key)
if (result == null) {
syncyronized (parsedScripts) {
result = parsedScripts.get(key)
if (result == null) {
result = genResult()
parsedScripts.put(key, result)
}
}
}
return result
==
DCL is about the protected resource, parsedScripts. Please try again.
The clue here it to look at what is being protected by the
synchronized keyword.
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On May 11, 2012, at 4:26 PM, Adam Heath wrote:
>>>>> @@ -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;
>>>>> + }
>>>>> + }
>>>>> }
>>>>> -
>
> This is DCL.
Oh no, you are wrong Adam: it would be DCL if scriptClass was a shared variable, which is not: it is a *local* variable.
Jacopo
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Adam Heath <do...@brainfood.com>.
On 05/11/2012 08:53 AM, Adam Heath wrote:
> On 05/11/2012 12:40 AM, Jacopo Cappellato wrote:
>> Hi Adam,
>>
>> please see inline:
>>
>> On May 11, 2012, at 4:52 AM, Adam Heath wrote:
>>
>>> On 04/20/2012 07:53 AM, jacopoc@apache.org 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) {
Jacopo: this is not DCL.
>>>> @@ -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;
>>>> + }
>>>> + }
>>>> }
>>>> -
This is DCL.
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Adam Heath <do...@brainfood.com>.
On 05/11/2012 12:40 AM, Jacopo Cappellato wrote:
> Hi Adam,
>
> please see inline:
>
> On May 11, 2012, at 4:52 AM, Adam Heath wrote:
>
>> On 04/20/2012 07:53 AM, jacopoc@apache.org 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);
>> }
>>
>
> Yeah, you are 100% right that your above is functionally equivalent (it fixes the bug too) but your is more concise and elegant. The only reason I implemented a custom check-then-act pattern (this is not a DCL!) is that I didn't notice that we were using a thread safe class: I know it may sound silly, but I was working on a similar fix for the freemarker code and in freemarker they were using an HashMap for the cache so I have implemented a similar code that I then copied over to OFBiz.
>
>> 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.
>
> The problem was that the same class was parsed by different threads and then added to the cache multiple times (of course each time overriding the same value as the key was the same): this is harmless in OFBiz but Freemarker interpreted this as if the class was changed and this triggered a complete clearing of cache; and, because of a bug in freemarker this could cause a deadlock (in freemarker).
>
> As I mentioned, I have sent a fix to Freemarker for this, but if you are interested to the details you can refer to:
>
> https://sourceforge.net/tracker/?func=detail&aid=3519805&group_id=794&atid=100794
> https://sourceforge.net/mailarchive/message.php?msg_id=29148020
Haven't yet looked at those urls, but just based on your description,
the only way to fix code that has broken locking is to wrap it in
synchronized. If you use non-locking techniques, the code being called
might still be run in parallel, which won't help the situation.
Let me look into this further before you this code is changed.
If I find out that there is a fixed freemarker(or, I come up with a
patch), then I assume we'd be for including that fixed freemarker. What
about backporting that to other branches(just the locking fix for
freemarker, plus whatever locking fix in ofbiz)?
.>> 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.
>
> I can update the trunk and release branches with code that actually takes into account that the parsedScript variable is an instance of a thread safe class... then you can review my changes.
>
>>
>> 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.
>
> "Java Concurrency In Action" is a great book, one of the best technical books I have ever read, and it is always in my desk.
>
>>
>> pps: It is my goal to remove all the DCL in ofbiz; I'd love it if others would help.
>
> I completely agree with this goal, but, as I mentioned above, the code I wrote above doesn't have anything to do with the double-checked locking anti-pattern. But I am with you completely when we talk about properly implementing concurrency in OFBiz... there is a lot of code that concerns me greatly (see for example ImageManagementServices.checkExistsImage(...)).
The code above that I originally quoted is doing DCL.
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
Hi Adam,
please see inline:
On May 11, 2012, at 4:52 AM, Adam Heath wrote:
> On 04/20/2012 07:53 AM, jacopoc@apache.org 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);
> }
>
Yeah, you are 100% right that your above is functionally equivalent (it fixes the bug too) but your is more concise and elegant. The only reason I implemented a custom check-then-act pattern (this is not a DCL!) is that I didn't notice that we were using a thread safe class: I know it may sound silly, but I was working on a similar fix for the freemarker code and in freemarker they were using an HashMap for the cache so I have implemented a similar code that I then copied over to OFBiz.
> 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.
The problem was that the same class was parsed by different threads and then added to the cache multiple times (of course each time overriding the same value as the key was the same): this is harmless in OFBiz but Freemarker interpreted this as if the class was changed and this triggered a complete clearing of cache; and, because of a bug in freemarker this could cause a deadlock (in freemarker).
As I mentioned, I have sent a fix to Freemarker for this, but if you are interested to the details you can refer to:
https://sourceforge.net/tracker/?func=detail&aid=3519805&group_id=794&atid=100794
https://sourceforge.net/mailarchive/message.php?msg_id=29148020
>
> 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.
I can update the trunk and release branches with code that actually takes into account that the parsedScript variable is an instance of a thread safe class... then you can review my changes.
>
> 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.
"Java Concurrency In Action" is a great book, one of the best technical books I have ever read, and it is always in my desk.
>
> pps: It is my goal to remove all the DCL in ofbiz; I'd love it if others would help.
I completely agree with this goal, but, as I mentioned above, the code I wrote above doesn't have anything to do with the double-checked locking anti-pattern. But I am with you completely when we talk about properly implementing concurrency in OFBiz... there is a lot of code that concerns me greatly (see for example ImageManagementServices.checkExistsImage(...)).
Kind regards,
Jacopo
Re: svn commit: r1328356 - /ofbiz/trunk/framework/base/src/org/ofbiz/base/util/GroovyUtil.java
Posted by Adam Heath <do...@brainfood.com>.
On 04/20/2012 07:53 AM, jacopoc@apache.org 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.