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
