Bug 28035 - java.rmi.server.UID does not return unique IDs
Summary: java.rmi.server.UID does not return unique IDs
Status: RESOLVED FIXED
Alias: None
Product: classpath
Classification: Unclassified
Component: classpath (show other bugs)
Version: 0.91
: P1 normal
Target Milestone: ---
Assignee: Audrius Meškauskas
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-06-15 02:59 UTC by David Pirkle
Modified: 2006-06-19 10:14 UTC (History)
1 user (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2006-06-18 20:50:41


Attachments
The fix (618 bytes, patch)
2006-06-18 20:52 UTC, Audrius Meškauskas
Details | Diff
Second fix (372 bytes, patch)
2006-06-19 10:11 UTC, Audrius Meškauskas
Details | Diff
The test case, showing, that the UID's are now really unique (obtaining the 4000 UID's in 20 threads) (552 bytes, text/plain)
2006-06-19 10:14 UTC, Audrius Meškauskas
Details

Note You need to log in before you can comment on or make changes to this bug.
Description David Pirkle 2006-06-15 02:59:35 UTC
A loop like the following will show duplicate ID's returned by the toString() method of java.rmi.server.UID:

  for (int i = 1; i <=20; ++i) {
    System.out.println(new UID().toString());
  }

Within the first few lines, you'll see an ID that appears twice in two consecutive lines.

I believe the problem is due to this line at the end of the constructor:

  count = uidCounter++;

Any time the clock advances, count and uidCounter are set to Short.MIN_VALUE.  The next iteration, the else branch is taken, and count will stay at Short.MIN_VALUE, since a postfix operator is used on uidCounter (see line above).  All subsequent iterations, count and uidCounter will continue to increment, until the clock ticks again, at which point the duplication reocccurs.

If I change the line above to the following, it seems to fix the problem:

  count = ++uidCounter;

By the way, shouldn't the whole constructor be synchronized?  Seems like there's potential for what's going on in the if branch to conflict with what's going on in the else branch (if you had 2 threads at each branch at the same time), but the if is not synchronized and the else is synchronized, so one can clobber the other.
Comment 1 Audrius Meškauskas 2006-06-18 20:50:41 UTC
The bug is confirmed. The general notes on the thread safety are not confirmed: after fixing the bug, I wrote a simple test with 20 threads and the duplicate UIDs are not returned. The bug will be fixed soon.
Comment 2 Audrius Meškauskas 2006-06-18 20:52:55 UTC
Created attachment 11692 [details]
The fix

This patch fixes the reported wrong behavior.
Comment 3 Audrius Meškauskas 2006-06-18 20:57:49 UTC
The fixing patch is commited. 
Comment 4 Jeroen Frijters 2006-06-19 07:16:57 UTC
Audrius, David is correct that the code is not thread safe. At the very least "last" should be made volatile, but it would be better (easier) to make the whole if/else construct synchronized.
Comment 5 Audrius Meškauskas 2006-06-19 10:11:43 UTC
Created attachment 11697 [details]
Second fix

This patch synchronizes on the whole constructor body. It must be applied subsequently after the first patch.
Comment 6 Audrius Meškauskas 2006-06-19 10:14:08 UTC
Created attachment 11698 [details]
The test case, showing, that the UID's are now really unique (obtaining the 4000 UID's in 20 threads)
Comment 7 Audrius Meškauskas 2006-06-19 10:14:54 UTC
I think, this time really OK. The proving test case is uploaded.