Jaroslav,
I'll do it.
-Dmitry
On 2012-10-15 20:17, Jaroslav Bachorik wrote:
> 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-
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
>>
>
--
Dmitry Samersoff
Java Hotspot development team, SPB04
* There will come soft rains ...
