Bojan Dolinar wrote:Both integers are carrying two roles now: semantics and synchronization.
That's not really a problem. In fact, we do it all the time. Every time you declare a method synchronized, the "this" object is carrying both roles. And it happens every time we use Collections.synchronizedXxx, or a Vector, Hashtable, or StringBuffer. I can't say I've ever heard of doing it on an immutable object though. Was that your point?
However, after some head scratching I figured that it should work. I wrote test and it passes.
It won't work. And it's near impossible to test multithreaded code. It might work a million times in a row but fail on the milliona-and-first.
It won't work because the following can happen:
In other words,
synchronized (X) is not atomic. We first have to read the value of X, then we have to obtain the lock on the object it points to. (I've kind of mixed references and objects in my example. I hope it's still clear.)
We've now had 2 increments, but X has only increased by 1.
So if both T1 and T2 read the value of X before either obtains the lock, and then T1 obtains the lock on that object, then changes X to point to a different object, that doesn't change the fact that T2 has already read the reference for the first object, and can lock it after T1 is done, and will never know that T1 has now said that we're actually using a different object.
If I were you, I'd change all those to AtomicIntegers.