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

[Alexander Scherbatiy]

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