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

[Alexander Scherbatiy]

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