Thanks David,
On 10/15/2012 04:19 AM, David Holmes wrote:
> 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.
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.
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
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-
>>>>>>
>>>>>
>>>
>>>
>>
