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

[Semyon Sadetsky]

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