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
