Synchronized New Object

My favorite “baddest piece of code encountered” is this one:

private static int clusterIdSeed = ...;
// ...
synchronized (new Object()) {
    int rdm = new Random(clusterIdSeed).nextInt();
    clusterId = String.valueOf((rdm<0)?(rdm*(-1)):rdm);
    clusterIdSeed = clusterIdSeed * 7;
}
// clusterId is used

It contains so much strange logic on just a few lines. It starts with:

synchronized (new Object()) {
	// ...
}

The obvious intention is that the code within the block should not be executed concurrently by multiple threads. Why? Perhaps the update of clusterIdSeed = clusterIdSeed * 7 is not atomic. Or Random.nextInt() is supposed to be not thread-safe (it is). In any case multiple threads will happily and concurrently enter the block because each thread will synchronize on a new instance of Object.

“But what about memory barriers?” I hear you calling? “At least synchronized will flush the caches so that other threads can pick up the values. volatile for the poor…” Well… not even that if you read Why is synchronized (new Object()) {} a no-op? on StackOverflow: According to the spec the “happens before” stuff does only apply to the new Object() part - a conforming JVM doesn’t need to flush the whole cache, it could flush only the new object.

The next interesting part:

    int rdm = new Random(clusterIdSeed).nextInt();

This spawns two questions:

• Why supply an explicit clusterIdSeed?

  The default constructor Random() is defined to get suitably distinct generators:

  This constructor sets the seed of the random number generator to a value very likely to be distinct from any other invocation of this constructor. JavaDoc Random

  But indeed - at the times of Java 1.3 (or so) Random() might have used something like System.currentTimeMillis() and in a very tight look two random generators might get the same seed. But the next point spoils this.

• Why is a new instance of Random created each time?

  Trading the static variable clusterIdSeed for a static Random instance would solve this. Also this would solve the previous point: Calling nextInt() twice on the same instance most probably will not generate the same number. Multithreading is not a problem because

  Instances of {@code java.util.Random} are threadsafe. JavaDoc Random

  If contention is a problem then wrapping the Random instance into a ThreadLocal would be better. And since Java 1.7 there is a ThreadLocalRandom.

Minor nitpick: rdm is a strange name for a Random instance: Most people (including me) would have used rnd as the abbreviation.

The next part:

	clusterId = String.valueOf((rdm<0)?(rdm*(-1)):rdm);

Also two things:

• To negate a value you can just use -rdm - multiplication is not necessary.
• The goal of the ternary operator is to get positive values. Just use Math.abs(int) for that. Then everybody known immediately what is required.

The last part:

	clusterIdSeed = clusterIdSeed * 7;

This line might be reason for the synchronized block. But it is also related to the new Random(clusterIdSeed) line (and its problems). Additionally I think this is a kind of “spillover thinking” with the usual recommendation to use primes when implementing hashCode(). See StackOverflow: Why use a prime number in hashCode?

OK… OK… I would have written the line more concisely like this – if I’m forced to do so:

	clusterIdSeed *= 7;

but that is – after all – really a minor thing.

Wrapping it up – what is a better and clearer solution? This code:

private static Random rnd = new Random();
// ...
clusterId = Integer.toString(Math.abs(rnd.nextInt()));