Possible atomicity violations when composing concurrent collections operations

[David Holmes]

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);
>