double-checked locking... in the JDK core?!

General Discussions
#1

While doing something completely unrelated, I ended up inadvertently browsing java.util.Random src, and noticed…


    private static Random randomNumberGenerator;

    private static synchronized Random initRNG() {
        Random rnd = randomNumberGenerator;
        return (rnd == null) ? (randomNumberGenerator = new Random()) : rnd;
    }

    public static double random() {
        Random rnd = randomNumberGenerator;
        if (rnd == null) rnd = initRNG();
        return rnd.nextDouble();
    }

Isn’t that an obtuse implementation of double-checked locking?

I thought out of order writes made the double-checked locking idiom all kinds of bad?

No doubt if it is a bug it’d be hideously complicated to detect it in any kind of unit test.

#2

Wasn’t double-checked locking fixed in Java 5 anyway?

#3

Wasn’t that usage fixed in the second memory model?

#4

yeah i think it was “fixed” in java 5 if you made the variable volatile… i.e.

private static volatile Random randomNumberGenerator;

so it does seem like a bug

#5

http://bugs.sun.com/view_bug.do?bug_id=6633229

Found it in the bug database.
Reported 5 years ago… Still not fixed.

#6

granted that their rationale for not fixing is pretty convincing… I would have thought that the fix was low impact/risk

#7

That should work


  private static Random randomNumberGenerator;
  private static boolean randomNumberGeneratorCreated=false;

    private static synchronized Random initRNG() {
        
		if(!randomNumberGeneratorCreated)
		{
		randomNumberGenerator = new Random();
		randomNumberGeneratorCreated=true;
		}
		return randomNumberGenerator;
    }

    public static double random() {
        Random rnd = randomNumberGenerator;
        if (!randomNumberGeneratorCreated) rnd = initRNG();
        return rnd.nextDouble();
    }
#8

The thing is, in this case, it doesn’t matter if more than one end up getting created.

#9

Maybe in some special cryptographic algorythm setup it may matter.

#10

If you need serious random numbers you shouldn’t use java.util.Random. That aside, worst case situation is a given caller might see two values that are slightly less decollerated than if this potential defect didn’t exist, but it cannot effect any meaningful computation. I promise, this isn’t a problem.

#11

java.util.Random is a LCG which is by definition not a cryptographic strength source of random numbers. Hell it doesn’t even pass the die hard tests and its not all that fast.

Its good enough for most things… But even my scientific code uses a far better implementation (yea and better than MT too).

Also does anything use these static things? I always create a random for each thread.

#12

You really should never use the same generator on more than one thread. Which generator? WELL?

#13

It may just be my slow mind today…but what exactly is the ‘bug’ here? What’s wrong with this double-checked locking?

EDIT: I could probably envision some race conditions, so that might be what the problem is here. Damocle’s code is even worse, because the boolean is set after the object is created, with a greater chance of a race condition.

The only fix I can see for this is to initialize the ‘rng’ in a static initialization block.

#14

Try this - http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

After the object is created may be just what the doctor ordered! :wink:

I much prefer this myself, and if done properly the class isn’t initialized until it’s needed anyway.

#15

The source I have for java.util.Random in JDK7 looks nothing like that and has no public static methods at all.

#16

Sorry, Math.

#17

The usage is safe – the RNG instance is private, and the reference never escapes, and the second check that does the actual creation synchronizes on it. You’ll notice that the second check returns the instance if had lost a race and another thread created it inbetween – this is an addition to the double-checked idiom that isn’t in the standard example. Since there is no other path that can create the RNG, there is no other race condition.

I prefer the static inner class idiom over that double-checking foofrah anyway.

#18

You sure? Looks like the standard example with the second if changed for a ternary operation to me.

#19

The standard double-checked locking is “if the reference is null, create it, and do nothing if it’s not null, then grab the reference again and return it”. This one is “if the reference is null, create it, and return it if it’s not null, then return that”. It’s subtly different, but in the latter case, the reference being returned is properly synchronized.

This stuff is subtle and tricky, and I’m by no means a concurrency guru, so I’m wrong, do point it out with a counterexample. Though like I mentioned before, I don’t bother with double-checked locking anyway, so it’s more academic curiosity.

At any rate, since you can’t control the seeding of the static RNG instance in Math, it’s not like a race condition would have much observable effect anyway.

#20

I now see the difference you mean, but I’m not sure it protects against out-of-order writes in the constructor. See this bug linked from one above - the random could potentially be used before the seed is initialised (AtomicLong) causing a NPE. The evaluation of that bug seems to suggest it can’t happen with the Sun JVM so who knows?

+1