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.
