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

[Stuart Marks]

Hi David,

Thanks for your notes and analysis. Good point about the temporary 
inconsistency between groupEntry and groupTable. With the change, there's a 
brief moment where a GroupEntry in the registered state can be observed to be 
absent from the groupTable, which wasn't possible before. I don't think this is 
a problem though. I can't find any places where operations on a GroupEntry need 
to look in the groupTable. (If they did exist, they'd have run into the same 
deadlock that I ran into in making this change.) Most paths through the code 
seem to first fetch an entry from groupTable and then operate on the entry. 
Somebody could remove and unregister the entry from groupTable between the 
fetch and the operation on the entry, but that can already occur in absence of 
this change.

In general the style of this code seems to be to lock individual data 
structures piecemeal, so I wouldn't be surprised if there are already cases 
where they can be inconsistent temporarily. For example, it seems like there 
ought to be some relationship between the members of idTable and groupTable, 
yet they are locked and updated independently through quite different code 
paths. This is dangerously close to a line of reasoning that goes like "it's 
already inconsistent, so making it a little more inconsistent doesn't hurt" 
which I detest. However, I don't have any better ideas at the moment.

Also note that switching from HM to CHM (regardless of what we do with the 
serialized form) has the same issue. Doing this will protect the maps 
themselves -- and avoid ConcurrentModificationException, the original cause of 
this problem -- but it won't preserve any consistency between the maps.

Thanks for mentioning startupLock. It seemed suspicious to me too, but I think 
addressing those issues is out of scope for this bugfix.

I'll talk about serialization more in my forthcoming reply to Alan.

s'marks

On 4/7/11 4:19 PM, David Holmes wrote:
> Hi Stuart,
>
> I can't answer your specific questions as I'm not familiar with the RMI
> infrastructure either. Taking the lock in writeObject and moving
> group.unregister out of the sync block to avoid deadlock seems reasonable. The
> main question to ask with such a move is whether the temporary inconsistency in
> state is something that can be observed and cause a problem.
>
> The lock ordering analysis seems reasonable.
>
> I do note that the startupLock protocol seems suspicious as there is a race
> with detection of the lock value. I assume init itself can never be invoked by
> more than one thread ;-) It may be that the object can only become accessible
> concurrently at some time during init (ie it gets published) in which case the
> protocol will work fine - but you really need a full understanding of the
> lifecycle of the objects to determine that.
>
> That aside, using ConcurrentHashMap would simplify the solution to the current
> problem. If you are concerned about the serialized form then you could define
> writeObject and readObject such that they convert between CHM and plain HM
> using the serialized-fields API.
>
> Cheers,
> David
>
> Stuart Marks said the following on 04/08/11 08:19:
>> 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/
>>
>> Briefly, the problem is incorrect synchronization of groupTable, a HashMap
>> field of an Activation object. The code mostly locks groupTable around any
>> access to it. However, no such locking is done during serialization. If the
>> groupTable is modified while it's being serialized,
>> ConcurrentModificationException occurs.
>>
>> The obvious fix would be to use ConcurrentHashMap instead of Hashmap and to
>> remove the external locking entirely. Unfortunately this will change the
>> serialized representation of the Activation object, which I'm not sure is
>> acceptable.
>>
>> Assuming that we can't change the serialized represenation, the alternative
>> approach would be to make sure that locking is done properly during
>> serialization. This is fairly easy to do by locking groupTable in a
>> writeObject() method. Unfortunately, this introduces a deadlock.
>>
>> This deadlock occurs because, with this modification, there are now paths
>> through the code that take locks in the opposite order. Specifically, the
>> addLogRecord() method locks the log object and then (via serialization and
>> the newly added writeObject() method) locks groupTable. However, the
>> unregisterGroup() method locks groupTable and calls
>> GroupEntry.unregisterGroup() which logs the event, which takes a lock on the
>> log.
>>
>> After some analysis, I've determined that the call to
>> GroupEntry.unregisterGroup() can be moved outside the synchronization of
>> groupTable. This removes the ordering problem.
>>
>> With these fixes in place (the state of the webrev above) I can get several
>> hundred successful test runs with neither ConcurrentModificationException nor
>> any deadlocks.
>>
>> Of course, that doesn't mean the change is correct. :-)
>>
>> Questions:
>>
>> 1. Is there a requirement that the serialized form of Activation remain
>> unchanged? If we can change it, we might as well just use ConcurrentHashMap
>> instead of HashMap.
>>
>> 2. Is my lock ordering analysis correct? I've pored over the code, but not
>> really being familiar with any RMI concepts, I'm not sure I have it right.
>> I've written this analysis into a big comment I've added to the code.
>>
>> 3. There is also a potential concurrency issue with idTable, which is used
>> similarly to groupTable. I haven't seen a test failure from it though. It
>> seems sensible to add a lock for it in Activation.writeObject() though. I
>> don't particularly like nesting the locks of idTable and groupTable, but
>> locking them separately would require serializing the Activation object field
>> by field instead of simply using defaultWriteObject(). Is this a reasonable
>> approach?
>>
>> Thanks for any advice or comments.
>>
>> s'marks
>>