Looks good to me.
Hopefully someone else will chime in too :)
Thanks,
David
On 15/10/2012 10: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-
