jmx-dev [PATCH] JDK-6809322: Missing notifications from javax.management.timer.Timer

[David Holmes]

On 15/10/2012 8:08 PM, Jaroslav Bachorik wrote:
> On 10/15/2012 04:19 AM, David Holmes wrote:
>> I think your changes now go further than needed. The original code uses
>> a dual synchronization scheme:
>>
>> a) it synchronizes most of the Timer methods
>> b) it also uses a thread-safe Hashtable
>>
>> This means that not all of the Timer methods need to be synchronized
>> because the only thread-safe action needed is the actual access to the
>> Hashtable in some methods.
>>
>> The flaw with the original code was simply that the iteration of the
>> Hashtable in notifyAlaramClock was not done in a thread-safe manner. I
>> believe this could be fixed simply by synchronizing on the Hashtable here:
>>
>> 1186         synchronized(timerTable) {
>>
>> with no need to change the type of the timerTable, nor the
>> synchronization on other Timer methods. You could alternatively
>> synchronize on the Timer itself - as you now do - provided all methods
>> of the Timer that mutate the Hashtable are themselves synchronized on
>> the timer.
>>
>> What you have is not incorrect though, and may remove unnecessary
>> synchronization in some cases (but increases the size of critical
>> sections in others).
>>
>> Also here:
>>
>> 165     volatile private int counterID = 0;
>>
>> there is no need to add volatile as counterID is only accessed within
>> synchronized methods.
>
> Yes, I see your point. I just want to ask - in cases of fixing issues
> like this the preferred way is to introduce minimal changes even if it
> means leaving the parts of the code sub-optimal? IMO, having dual
> synchronization scheme might be considered as sub-optimal as it makes it
> more difficult to see the author's intentions.

Optimal depends on your evaluation criteria. The original design may 
have been done with performance in mind and a view to minimising 
critical sections. Without knowing what the original design criteria 
was, and unless you are fixing a problem caused by key aspects of that 
design, then minimal changes should be favoured.

> But I am fine with leaving the Hashtable intact and just synchronizing
> the iteration part correctly - it resolves the issue.
>
> The update webrev is available at
> http://btrace.kenai.com/webrevs/JDK-6809322/webrev.v4

I'm not sure the comment is needed in that form. Hashtable is 
snchronized internally but you need to use external synchronization when 
iterating through it.

David

> Regards,
>
> -JB-
>
>>
>> David
>> -----
>>
>> On 12/10/2012 11:14 PM, Jaroslav Bachorik wrote:
>>> The updated webrev is now at http://btrace.kenai.com/webrevs/JDK-6809322/
>>>
>>> I am sorry for this chaos with webrev locations but its not that easy to
>>> work efficiently without an OpenJDK username :/
>>>
>>> -JB-
>>>
>>> On 10/12/2012 09:47 AM, Jaroslav Bachorik wrote:
>>>> On Fri 12 Oct 2012 04:44:31 AM CEST, David Holmes wrote:
>>>>> Hi Jaroslav,
>>>>>
>>>>>
>>>>> On 11/10/2012 6:07 PM, Jaroslav Bachorik wrote:
>>>>>> Dmitry has put the webrev on the public CR -
>>>>>> http://cr.openjdk.java.net/~dsamersoff/sponsorship/jbachorik/JDK-6809322-v2/
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> -JB-
>>>>>>
>>>>>> On 10/10/2012 04:17 PM, Jaroslav Bachorik wrote:
>>>>>>> I am looking for a review and a sponsor.
>>>>>>>
>>>>>>> The issue is about some javax.management.timer.Timer notifications
>>>>>>> not
>>>>>>> being received by the listeners if the notifications are generated
>>>>>>> rapidly.
>>>>>>>
>>>>>>> The problem is caused by ConcurrentModificationException being
>>>>>>> thrown -
>>>>>>> the exception itself is ignored but the dispatcher logic is skipped.
>>>>>>> Therefore the currently processed notification gets lost.
>>>>>
>>>>> Can you point out where exactly in the code the exception is thrown
>>>>> and caught. I'd like to understand the problem better.
>>>>
>>>> The CME is thrown in Timer.notifyAlarmClock() method in this case - but
>>>> may happen in other places as well.
>>>>
>>>> Actually, in some places the access to the timerTable map is
>>>> synchronized while in others it isn't. While switching the Hashtable
>>>> for ConcurrentHashMap resolves this particular issue it might be
>>>> beneficial to correct the partial synchronization instead.
>>>>
>>>>>
>>>>>>>
>>>>>>> The CME is thrown due to the Timer.timerTable being iterated over
>>>>>>> while
>>>>>>> other threads try to remove some of its elements. Fix consists of
>>>>>>> replacing the Hashtable used for Timer.timerTable by
>>>>>>> ConcurrentHashMap
>>>>>>> which handles such situations with grace.
>>>>>
>>>>> Be aware that it may also give surprising results as removal is no
>>>>> longer synchronized at all with processing. So it could now appear
>>>>> that a notification is processed after a listener has been removed.
>>>>
>>>> Indeed, the CME is the symptom of the out-of-order processing - the
>>>> removal method is synchronized on (Timer.this) while the
>>>> notifyAlarmClock() method, processing the notifications, runs
>>>> unsynchronized.
>>>>
>>>> Thanks for pointing this out. I will have something to think about.
>>>>
>>>> -JB-
>>>>
>>>>>
>>>>> David
>>>>> -----
>>>>>
>>>>>>> The patch webrev is available @
>>>>>>> https://jbs.oracle.com/bugs/browse/JDK-6809322
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> -JB-
>>>>>>>
>>>>>>
>>>>
>>>>
>>>
>