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

[David Holmes]

Hi Jaroslav,

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.

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-
>>>>>
>>>>
>>
>>
>