Possible atomicity violations when composing concurrent collections operations

[Phil Race]

 > I think the main point is that if the first thread to create the map 
also populates it with some entries,
 > then they will be lost when the second map is created and installed.
 > Of course if the entries are idempotent then it is a non-issue (just 
as with the two races that Vitaly addressed).

Exactly the case and that's what I tried to describe already. Its a 
non-issue, not
in need of any fix.

-phil.

On 8/5/2012 5:03 PM, David Holmes wrote:
> Re-directing this thread to jdk8-dev ...
>
> On 3/08/2012 7:16 AM, Phil Race wrote:
>> On 8/2/2012 11:52 AM, Yu Lin wrote:
>>> My name is Yu Lin. I'm a Ph.D. student in the CS department at
>>> UIUC. I'm currently doing research on mining Java concurrent library
>>> misuses. I found some uses of concurrent collections in OpenJDK7u may
>>> result in potential atomicity violation bugs or harm the performance.
>> ....
>>
>>> Another possible atomicity vioaltion may happen at line 892 in
>>> jdk/src/share/classes/sun/font/FileFontStrike.java:
>>>
>>> L891 if (boundsMap == null) {
>>> L892 boundsMap = new ConcurrentHashMap<Integer, Rectangle2D.Float>();
>>> L893 }
>>> // some<key, value> pairs are put into "boundsMap"
>>>
>>> A possible buggy scenario is both threads T1 and T2 finds "boundsMap"
>>> is null at the same time (line 891). After T1 creates a new empty map
>>> and puts some<key, value> pairs into "boundsMap", T2 also creates an
>>> empty map and writes to "boundsMap". Then the<key, value> pairs put
>>> by T1 will be lost. If this scenario may happen and lead to bug, we
>>> have to add synchronization when initializing "boundsMap".
>>
>> I haven't looked at the rest but I'll comment on that one.
>> This is deliberate. The tradeoff is all uses being synchronized versus
>> the small chance that two threads try to create this map at the same
>> time, which really doesn't matter. Its just a tiny bit of lost work.
>> We could have a debate about how uncontended synchronization
>> is cheap but adding it somewhat defeats the purpose of using
>> ConcurrentHashMap. Also if there are two threads active here,
>> meriting the synchronization then the lock probably is contended ..
>
> I think the main point is that if the first thread to create the map 
> also populates it with some entries, then they will be lost when the 
> second map is created and installed. Of course if the entries are 
> idempotent then it is a non-issue (just as with the two races that 
> Vitaly addressed).
>
> David
> -----
>
>
>> Also I am not sure how your patch below would work.
>> Synchronizing on (null) isn't likely to fix anything.
>> Instead you just introduced a NullPointerException.
>>
>> Finally jdk8-dev is probably the "main" list these days.
>>
>> -phil.
>>
>>> I append a patch to show how to fix the above four problems. I can
>>> also list
>>> all of such code I found if you need.
>>>
>>> Regards,
>>> Yu
>>>
>>> ===============================================================
>>>
>>> diff -r 7daf4a4dee76 src/share/classes/sun/font/FileFontStrike.java
>>> --- a/src/share/classes/sun/font/FileFontStrike.java Mon May 07
>>> 14:59:29 2012 -0700
>>> +++ b/src/share/classes/sun/font/FileFontStrike.java Thu Aug 02
>>> 11:50:46 2012 -0500
>>> @@ -887,9 +887,10 @@
>>> * up in Java code either.
>>> */
>>> Rectangle2D.Float getGlyphOutlineBounds(int glyphCode) {
>>> -
>>> - if (boundsMap == null) {
>>> - boundsMap = new ConcurrentHashMap<Integer, Rectangle2D.Float>();
>>> + synchronized (boundsMap) {
>>> + if (boundsMap == null) {
>>> + boundsMap = new ConcurrentHashMap<Integer,
>>> Rectangle2D.Float>();
>>> + }
>>> }
>>>
>>> Integer key = Integer.valueOf(glyphCode);
>>