JDK 8 RFC 6470700: Math.random() / Math.initRNG() uses "double checked locking"

[Vitaly Davidovich]

There's a significant difference here: Random reads the field into a local
and then operates only on the local.  Looking at the code, I only see one
possible (bizarre) circumstance where you can hit NPE.  If code was
transformed to:

static double random() {
      Random rnd = randomNumberGenerator; // assume null is read
      if (randomNumberGenerator == null) ... // note the field is re-read
instead of local, and assume it's not null now
     return rnd.nextDouble(); // NPE
}

I don't see a case where any sane compiler would do this but I think it's
legal transformation given that the field is non-volatile and single
threaded execution would not detect difference.  Unless phantom reads
cannot be introduced even for plain fields, in which case I don't see how
NPE can occur.

Having said that, David's suggestion is cleaner anyway.

Sent from my phone
On Aug 21, 2013 8:36 PM, "Steven Schlansker" <stevenschlansker at gmail.com>
wrote:

>
> On Aug 21, 2013, at 4:37 PM, Brian Burkhalter <brian.burkhalter at oracle.com>
> wrote:
>
> > With respect to this issue
> >
> > http://bugs.sun.com/view_bug.do?bug_id=6470700
> >
> > the code of concern from java.lang.Math is
> >
> > 701    private static Random randomNumberGenerator;
> > 702
> > 703    private static synchronized Random initRNG() {
> > 704        Random rnd = randomNumberGenerator;
> > 705        return (rnd == null) ? (randomNumberGenerator = new Random())
> : rnd;
> > 706     }
> >
> > 731    public static double random() {
> > 732        Random rnd = randomNumberGenerator;
> > 733        if (rnd == null) rnd = initRNG();
> > 734        return rnd.nextDouble();
> > 735    }
> >
> > where the class variable "randomNumberGenerator" is not used anywhere
> else in the class. The complaint is that this is an instance of the broken
> double-checked locking pattern. While at first glance this might appear to
> be the case, it does not seem so to me. It looks more like an attempt to
> avoid calling a synchronized method if "randomNumberGenerator" has already
> been initialized.
>
>
> This looks very much like the "Broken multithreaded version" from
> http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
>
> // Broken multithreaded version
> // "Double-Checked Locking" idiom
> 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...
>   }
>
>
> "Unfortunately, that code just does not work in the presence of either
> optimizing compilers or shared memory multiprocessors."
>
>