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
