<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