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

[Stuart Marks]

Hi all,

Please review an updated webrev for this bug:

http://cr.openjdk.java.net/~smarks/reviews/6896297/webrev.1/

Using ConcurrentHashMap is much nicer in many ways, and it seems to resolve the 
JCK failures at least as well as the previous fix did. This does nothing to 
remedy the issue of snapshot consistency, which seems to be covered by 4945726, 
which we'll postpone (again) to a future release.

s'marks

On 4/14/11 4:01 PM, Stuart Marks wrote:
> Hi Peter,
>
> Thanks for your comments on this issue. I understand from Alan that you have
> some expertise in RMI; it's most welcome here.
>
> It's good to hear that serialization compatibility isn't that big of a deal.
> This makes things easier. It would seem reasonable to investigate replacing
> HashMap with ConcurrentHashMap, which would remove a fair bit of the external
> locking, and reduce or eliminate the need for a complex locking hierarchy along
> with a big comment that describes it. This will also make it easier to retrofit
> a coarser locking strategy if necessary; see below.
>
> Converting instances of HM to CHM if necessary upon deserialization is probably
> a good idea and isn't too difficult.
>
> The larger issue of snapshot consistency is indeed troubling. I'd agree that
> the ConcurrentModificationException is a symptom of a larger problem and that
> making it go away (either with a different locking strategy or using CHM) is
> mostly merely suppressing the symptom without addressing the larger issue. The
> immediate need, though, is to fix the JCK failure, so I suspect what I'll need
> to do is to push a fix for the CME and handle the resolution of any larger
> issue separately.
>
> I'll investigate an alternative fix that uses CHM instead of modified external
> locking, and I'll post an updated webrev.
>
> s'marks
>
>
> On 4/13/11 11:14 PM, Peter Jones wrote:
>> Stuart,
>>
>> A couple of comments so far:
>>
>> I don't think that there would be a serialization compatibility problem with
>> changing Activation.groupTable to have a ConcurrentHashMap instead of a
>> HashMap. The field's declared type is Map, so the deserialization process
>> would be able to assign either class to the field, and the code doesn't
>> otherwise care about the Map implementation class. An rmid's state created
>> with CHM could not be recoverable with JDK 1.4, of course, but I don't think
>> that that's a concern. The fix would not help an rmid whose state gets
>> recovered from an earlier version created with HM, unless a replacement
>> (HM->CHM) was also done upon deserialization.
>>
>> More troubling I think is the higher-level issue suggested by this
>> ConcurrentModificationException. Thread B (in the 6896297 Evaluation) is
>> attempting to write a "snapshot" of the entire persistent state of the
>> Activation instance, which presumably should be consistent with future
>> "updates" written to the log, while Thread A is concurrently attempting to
>> mutate that state (before writing an update for its mutation). It seems
>> highly doubtful that potentially including an uncertain amount of this
>> concurrent mutation in the written snapshot is safe. I might expect a coarser
>> lock to be used across all such mutations, together with their associated log
>> writes (at which point groupTable wouldn't need to be concurrent).
>>
>> The approach at your webrev below seems OK to address the immediate
>> ConcurrentModificationException, but (unless I'm missing something) the
>> higher-level issue with snapshot correctness described above would remain.
>>
>> Cheers,
>>
>> -- Peter
>>
>> P.S. This seems somewhat related to
>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4945726
>>
>>
>> On Apr 7, 2011, at 6:19 PM, 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/
>>>
>>> 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
>>>
>>