We were writing a small, low level code to provide sequence number generator. It was backed by database sequences, and it worked fine, or at least we thought so…
All unit tests passed fine. – We wrote some multi-thread testNG tests. So we were quite sure, everything is fine, until we got a bug report… They said we have duplications. Well the first reaction was “no way”. But the error came out again and again. To isolate the problem, we wanted to be sure that our code is fine, so we wrote a stress test: 10 threads, 20 iterations of getting 1 million numbers and storing it into a unique field of database table.
And the stress test started. We event started it 3 times: 3 separate VMs * 20 iterations simultaneously consuming 10 threads. And it ran… ran… for about 5h… and crash… oups, unique constraint violation… 
Well there is no way to reproduce this kind of error in debug mode, at least not until you know where the error might be. So we started an “eye debug” session… and after, about half an hour we got it! The code was:
private volatile Long last = -1l;
public final V getNext() {
if (!initialized) {
init();
}
synchronized (last) {
if( last % increment == 0 ) {
dbSequenceGetNextValue();
}
last++;
return (V)last;
}
}
Well, it looks fine… or at least at first glance. But this code is nasty.
The problem is with autoboxing. Well not exactly with autoboxing, but how we used it.
If you look at the line #11, that would be actually:Long.valueOf(last.intValue()++)
And that is the problem! We have a new object here! And we were using that variable to synchronize our block! Ouch.
Now the correct version is:
private volatile Long last = -1l;
//synch object
private volatile Object getNextSynch = new Object();
public final V getNext() {
if (!initialized) {
init();
}
synchronized (getNextSynch) {
if( last % increment == 0 ) {
dbSequenceGetNextValue();
}
last++;
return (V)last;
}
}
Of course we could use AtomicLong too, but we do not need that complexity…
Moral of the story:
- Always think twice, when you are coding thread-safe, synchronized, parallel, etc…
- Instant sugar is evil!
p.s.: Stress tests still running… 
Hope we’ll get a result by tomorrow.
