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

[Jaroslav Bachorik]

Thank you guys for the reviews! I would like to kindly ask someone with
the committer rights to sponsor this fix and push the change.


Thanks!

-JB-

On 10/15/2012 04:15 PM, Rickard Bäckman wrote:
> Looks good!
> 
> /R
> 
> On Oct 15, 2012, at 2:14 PM, Jaroslav Bachorik wrote:
> 
>> On 10/15/2012 01:45 PM, David Holmes wrote:
>>> 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.
>>
>> Ok, it's gone. http://btrace.kenai.com/webrevs/JDK-6809322/webrev.v5/
>>
>>>
>>> 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-
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>
>