<Swing Dev> [9] Review Request for 8130735: javax.swing.TimerQueue: timer fires late when another timer starts

[Alexander Scherbatiy]

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