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

[Semyon Sadetsky]

Yes, but then it will not very readable. I included your 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.
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>