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

[Semyon Sadetsky]

Alexander,

Thank you for the contribution. The final update:
http://cr.openjdk.java.net/~ssadetsky/8130735/webrev.02/

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