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