Possible atomicity violations when composing concurrent collections operations

[Phil Race]

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 ..

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