Possible atomicity violations when composing concurrent collections operations

[Ludwig, Mark]

I agree that these are not in need of fixing.

BTW, nice name for this topic, Mr. Race.  :-)

> From: Phil Race
> Sent: Monday, August 06, 2012 12:03 PM
> To: David Holmes
> Cc: jdk8-dev at openjdk.java.net; Yu Lin
> Subject: Re: Possible atomicity violations when composing concurrent
> collections operations
> 
>  > 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);
> >>