advice & review requested for 6896297, race condition in rmid causing JCK failure

[Stuart Marks]

On 4/11/11 7:46 AM, Alan Bateman wrote:
> Stuart Marks wrote:
>> Hi Core-Libs developers,
>>
>> I'd like to solicit some advice and discussion about this bug and a potential
>> fix I'm cooking for it. Here is the bug report; it contains details about the
>> problem and my analysis of it:
>>
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6896297
>>
>> and here's a webrev of the fix I'm working on:
>>
>> http://cr.openjdk.java.net/~smarks/reviews/6896297/webrev.0/

> I haven't worked on RMI and so I don't have a good insight into the
> implementation. I think (and Peter Jones can correct me) that changing the
> serialized form would just inhibit recovery of rmid from an existing log file.
> As David said, you could switch to CHM and implement readObject/writeObject to
> serialize/deserialize in the current form. That said, I looked your webrev and
> the changes look reasonable to me. I don't see anything obviously wrong with
> moving the unregisterGroup out of the synchronized block.

I didn't see anything obviously wrong either... my hope (or fear) was that 
somebody would point out something non-obviously wrong. :-)

In a face-to-face conversation, Joe Darcy pointed out to me that serialization 
compatibility is over the types of the fields declared in the class, not the 
actual serial data in the object stream. The groupTable and idTable fields are 
both Maps, so we could change the implementation from HashMap to 
ConcurrentHashMap and it would be compatible with any implementation that has 
CHM, i.e. 1.5 or later. (It would probably be wise to have readObject 
reconstitute the tables as CHM in case they encountered HM in the serialized form.)

The alternatives seem to be as follows:

1. Convert groupTable and idTable to ConcurrentHashMap and remove external 
synchronization, and otherwise make minimal changes to serialization code. This 
puts a CHM into the object stream, which should be compatible with anything 
using JDK 1.5 or later as noted above.

2. Convert groupTable and idTable to ConcurrentHashMap and remove external 
synchronization, and provide readObject/writeObject implementations to place 
ordinary HashMaps (as opposed to CHMs) in the object stream. This serialized 
form is more compatible, but it requires more serialization code to be 
developed, tested, and maintained.

3. Adjust the locking strategy to prevent ConcurrentModificationException but 
otherwise make no changes to serialization or data structures. This is what 
I've posted in the webrev.

The key issue seems to be whether we want to preserve compatibility of the 
serialized form. If we care about compatibility of the serialized form, #3 is 
probably the safest. If somebody comes along and tells us that we don't have to 
worry about serial compatibility of this stuff at all, then #1 is probably the 
best since it simplifies the code a lot.

In the absence of any further information, I'm inclined to go with #3, since 
I've already developed and tested it and it's basically ready to go.

Any further thoughts, anybody?

s'marks