<Swing Dev> [9] Review Request for 8130735: javax.swing.TimerQueue: timer fires late when another timer starts
Alexander Scherbatiy
alexandr.scherbatiy at oracle.com
Thu Jul 23 14:55:00 UTC 2015
The test from the bug report does not use timer.stop(). Is it
possible to write a test with timer.start()/stop() to reproduce the
original issue?
Thanks,
Alexandr.
On 7/22/2015 11:56 AM, Semyon Sadetsky wrote:
> Yes, but then it will not very readable. I included you change with
> some comments http://cr.openjdk.java.net/~ssadetsky/8130735/webrev.03/.
>
> --Semyon
>
> On 7/22/2015 11:30 AM, Alexander Scherbatiy wrote:
>> On 7/22/2015 11:02 AM, Semyon Sadetsky wrote:
>>> Alexander,
>>>
>>> Thank you for the contribution. The final update:
>>> http://cr.openjdk.java.net/~ssadetsky/8130735/webrev.02/
>>
>> It looks like the runningTimer is never null so delayedTimer ==
>> runningTimer check always includes the delayedTimer to null check.
>>
>> Thanks,
>> Alexandr.
>>
>>>
>>> --Semyon
>>>
>>> On 7/21/2015 2:31 PM, Alexander Scherbatiy wrote:
>>>> On 7/21/2015 1:40 PM, Semyon Sadetsky wrote:
>>>>> In the run() the delayed timer is obtained from timer, so it is
>>>>> always the resulting delayed timer.
>>>> Just to use the delayed timer from the queue and not from the
>>>> timer?
>>>>
>>>> TimerQueue.run(){
>>>> DelayedTimer delayedTimer = queue.take(); // note that this
>>>> delayed timer is not the same as timer.delayedTimer
>>>> delayedTimer.getTimer().lock();
>>>> if(delayedTimer.removed){
>>>> // skip it
>>>> }
>>>> // ...
>>>> delayedTimer.getTimer().unlock();
>>>> }
>>>>
>>>> Thanks,
>>>> Alexandr.
>>>>
>>>>
>>>>> How do you propose to detect that timer was renewed exactly during
>>>>> the time interval between the timer is taken from the queue and
>>>>> the lock is captured in the run() thread?
>>>>>
>>>>> On 7/21/2015 12:53 PM, Alexander Scherbatiy wrote:
>>>>>> On 7/21/2015 11:36 AM, Semyon Sadetsky wrote:
>>>>>>> Hi Alexander,
>>>>>>>
>>>>>>> The remove() method set delayedTimer field to null, and it is
>>>>>>> checked in run() to be not null. So it acts in the same way as
>>>>>>> flag you've proposed.
>>>>>>>
>>>>>>> The issue is about how to detect consequent remove() ans add()
>>>>>>> in run(). If you use some flag in remove() then you need to
>>>>>>> clear it in add()
>>>>>> I suppose that you are talking about some flags in Timer. The
>>>>>> proposed change suggests to add the flag to the DelayedTimer. So
>>>>>> after stopping a timer (timer.delayedTimer = null) the
>>>>>> delayedTimer taken from the queue contains information that it
>>>>>> has been removed.
>>>>>>
>>>>>> Thanks,
>>>>>> Alexandr.
>>>>>>
>>>>>>> because after the add() the timer should run normally with the
>>>>>>> new period. And you cannot clear such flag in the first
>>>>>>> consequent run() after add() because you cannot determine the
>>>>>>> moment the add() was called. So it is just the same problem.
>>>>>>>
>>>>>>> --Semyon
>>>>>>>
>>>>>>> On 7/21/2015 10:35 AM, Alexander Scherbatiy wrote:
>>>>>>>>
>>>>>>>> What about to add removed flat to DelayedTimer? Something like:
>>>>>>>> TimerQueue.removeTimer(Timer timer){
>>>>>>>> timer.lock();
>>>>>>>> // ...
>>>>>>>> timer.delayedTimer.removed = true;
>>>>>>>> queue.remove(timer.delayedTimer);
>>>>>>>> timer.delayedTimer = null;
>>>>>>>> timer.unlock();
>>>>>>>> }
>>>>>>>>
>>>>>>>> TimerQueue.run(){
>>>>>>>> DelayedTimer delayedTimer = queue.take();
>>>>>>>> delayedTimer.getTimer().lock();
>>>>>>>> if(delayedTimer.removed){
>>>>>>>> // skip it
>>>>>>>> }
>>>>>>>> // ...
>>>>>>>> delayedTimer.getTimer().unlock();
>>>>>>>> }
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Alexandr.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 7/17/2015 12:28 AM, Sergey Bylokhov wrote:
>>>>>>>>> Hi, Semyon.
>>>>>>>>> I see that the chance to reproduce the problem is very very
>>>>>>>>> small, because we should call addTimer, when we are at lines
>>>>>>>>> 171/172. So the bug is about really small timings. So the
>>>>>>>>> related question: Is it possible in the fixed version to call
>>>>>>>>> addTimer when we remove DelayedTimer from the queue via
>>>>>>>>> queue.take(), but before we assign its value to the runningTimer?
>>>>>>>>>
>>>>>>>>> On 14.07.15 12:51, Alexander Zvegintsev wrote:
>>>>>>>>>> still looks good to me.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> Alexander.
>>>>>>>>>>
>>>>>>>>>> On 07/14/2015 12:41 PM, Semyon Sadetsky wrote:
>>>>>>>>>>> Hi Alexander,
>>>>>>>>>>>
>>>>>>>>>>> I added the double check
>>>>>>>>>>> :http://cr.openjdk.java.net/~ssadetsky/8130735/webrev.01/
>>>>>>>>>>>
>>>>>>>>>>> --Semyon
>>>>>>>>>>>
>>>>>>>>>>> On 7/13/2015 1:24 PM, Alexander Zvegintsev wrote:
>>>>>>>>>>>> Hello Semyon,
>>>>>>>>>>>>
>>>>>>>>>>>> the fix looks good to me.
>>>>>>>>>>>>
>>>>>>>>>>>> P.S. Just a side note, as I can see we could possibly start
>>>>>>>>>>>> two threads instead of one in startIfNeeded():
>>>>>>>>>>>>
>>>>>>>>>>>> 96 void startIfNeeded() {
>>>>>>>>>>>> 97 if (! running) {
>>>>>>>>>>>> 98 runningLock.lock();
>>>>>>>>>>>> 99 try {
>>>>>>>>>>>> 100 final ThreadGroup threadGroup =
>>>>>>>>>>>> AppContext.getAppContext().getThreadGroup();
>>>>>>>>>>>> 101
>>>>>>>>>>>> AccessController.doPrivileged((PrivilegedAction<Object>) ()
>>>>>>>>>>>> -> {
>>>>>>>>>>>> 102 String name = "TimerQueue";
>>>>>>>>>>>> 103 Thread timerThread = new
>>>>>>>>>>>> ManagedLocalsThread(threadGroup,
>>>>>>>>>>>> 104 this, name);
>>>>>>>>>>>>
>>>>>>>>>>>> !running check is missing after try. It is not the case
>>>>>>>>>>>> with current code base, but it may be changed in future.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>> Alexander.
>>>>>>>>>>>>
>>>>>>>>>>>> On 07/09/2015 08:08 PM, Semyon Sadetsky wrote:
>>>>>>>>>>>>> Hello,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Please review fix for JDK9:
>>>>>>>>>>>>>
>>>>>>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8130735
>>>>>>>>>>>>> webrev:
>>>>>>>>>>>>> http://cr.openjdk.java.net/~ssadetsky/8130735/webrev.00/
>>>>>>>>>>>>>
>>>>>>>>>>>>> The root cause is the setting larger expiration time for
>>>>>>>>>>>>> the timer which is already inserted into the delay queue.
>>>>>>>>>>>>> So all timers behind the timer cannot be executed earlier
>>>>>>>>>>>>> than its expiration time. This happens very rare only for
>>>>>>>>>>>>> repeated timers and only if user uses the Swing timer API
>>>>>>>>>>>>> inaccurately (call start() without stop()).
>>>>>>>>>>>>> The fix eliminates this possibility by introducing a check
>>>>>>>>>>>>> if the timer was already restarted concurrently.
>>>>>>>>>>>>> It is difficult to write test because I could not reliably
>>>>>>>>>>>>> reproduce the issue for a reasonable time.
>>>>>>>>>>>>>
>>>>>>>>>>>>> --Semyon
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Best regards, Sergey.
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the swing-dev
mailing list