Locking Gone Wrong

Sometimes you need to learn the hard way. Imagine the following method in your code that performs a synchronized operation like this:

The intention of the mayLock-method is to do some calculations (represented by the Thread.sleep method) guarded by the input parameter, e.g. some UserID. When you execute the code on a multicore CPU, the output should be something like:

17:14:39.905 [ForkJoinPool.commonPool-worker-2] INFO LockTest – entering method 127
17:14:39.905 [ForkJoinPool.commonPool-worker-1] INFO LockTest – entering method 127
17:14:39.913 [ForkJoinPool.commonPool-worker-2] INFO LockTest – entering lock ForkJoinPool.commonPool-worker-2
17:14:41.915 [ForkJoinPool.commonPool-worker-1] INFO LockTest – entering lock ForkJoinPool.commonPool-worker-1

Based on the timestamp, you can say, that the locking mechanic based on the argument works: about two seconds after the first Thread, the second Thread enters the synchronized section. Now changing the input parameter to 128 results into the following output:

17:20:33.849 [ForkJoinPool.commonPool-worker-1] INFO LockTest – entering method 128
17:20:33.849 [ForkJoinPool.commonPool-worker-2] INFO LockTest – entering method 128
17:20:33.858 [ForkJoinPool.commonPool-worker-1] INFO LockTest – entering lock ForkJoinPool.commonPool-worker-1
17:20:33.858 [ForkJoinPool.commonPool-worker-2] INFO LockTest – entering lock ForkJoinPool.commonPool-worker-2

That’s right. The method is executed right away without waiting for the lock. The reason behind this is Java’s autoboxing and the IntegerCache. Autoboxing is a convenient way of interoperating primitive int‘s and Integer-objects so you can write code like this:

When a primitive int is autoboxed into an Integer-object, the JVM first checks if the target value is within the range of the IntegerCache which is, by default between -127 and 127. That means the following code is valid:

Because 127 is within the bounds of the cache i refers to the same object as j. Let’s change to 128 now:

Because 128 is outside of the cache bounds, a new Integer object is created each time you do autoboxing (same goes for all values greater 127 and lower -127) and is therefore no good candidate for acquiring locks. Although you can increase the cache size as the Javadoc states, this solution does not scale very well.

/**
 * Cache to support the object identity semantics of autoboxing for values between
 * -128 and 127 (inclusive) as required by JLS.
 *
 * The cache is initialized on first usage.  The size of the cache
 * may be controlled by the {@code -XX:AutoBoxCacheMax=} option.
 * During VM initialization, java.lang.Integer.IntegerCache.high property
 * may be set and saved in the private system properties in the
 * sun.misc.VM class.
 */

In addition, you don’t know what parts in your code are locking on that Integer too, especially when you are working with third party libraries.

You might be tempted to switch to Strings, but something similar applies for Strings and the internal StringPool. Locking may work if the String is inside the StringPool but is not guaranteed to. So how to circumvent this?

Following the rule “first make it work – then make it fast“, we could synchronize the whole mayLock-method. Actually, this would work in many environments, but when you’re looking for performance locking on class-level is not an option. So my suggestion here is to create a Map<Integer,Object> which stores an Object you can synchronize on or a Map<Integer,java.util.concurrent.locks.Lock> if you need finer control over the locking process.

Since we’re using a Map with a hashing algorithm, Integer can safely be used as a key. The computeIfAbsent-method of the ConcurrentHashMap guarantees, that it returns the same object even if multiple Threads are accessing the method at the same time. When we run the test again, we see that the lock is correctly used.

However the Map needs to be maintained, e.g. we have to manually remove the key-value pairs once we’re done. Switching to a WeakHashMap that stores it’s keys with a weak reference (and is garbage collected when the key is no longer referenced) is not an option because of two reasons: a) this class is not threadsafe and b) Integers within the range of the IntegerCache will never be removed because they’re “hard” referenced.

As you can see, getting multithreading right is hard. Or simply put

99% of multithreaded Java code is wrong. The other 1% is written by Doug Lea and Brian Goetz.

Advertisements

2 thoughts on “Locking Gone Wrong”

  1. Never knew of that autoboxing of Integers and ints – but it makes sense – even though (imho) it would be more useful if every number (Byte, Integer, Long, maybe even Float and Double..) would be mapped to their Object counter part – so that (new Integer(127))==(new Integer(127)) is true. Could be realized if every constructer-call of a number-object as well as the autoboxing-magic is not creating a number but ALWAYS getting a reference to the same number-object in a concurrent way for -MAX_VALUE to MAX_VALUE (request of a pooled number->return the reference to it. new number->pool it and return a reference to it). Could be handled with a hash based map in the background – just like your approach with the locks. Very interesting and pleasant to read article!

  2. As mentioned in the post, you can control the size of the cache. However this comes with a price, which is memory-consumption. And for obvious reasons this caching only applies to integers, longs and bytes.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s