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