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