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

Alexander Scherbatiy alexandr.scherbatiy at oracle.com
Fri Jul 24 09:45:34 UTC 2015


   The fix looks good to me.

   Thanks,
   Alexandr.

On 7/23/2015 6:26 PM, Semyon Sadetsky wrote:
> yes, the reporter wanted to make issue more frequent, but actually 
> skipping the stop() makes the real issue not reproducible at all.
> The issue exists only for consequent start()/stop() calls and it's 
> very rare. Since it requires small timings there is a chance to catch 
> delay from the scheduler instead of the real problem.
> As I mentioned in the initial message it is hard to write a stable 
> test scenario, so the bug is labeled noreg-hard.
>
> --Semyon
>
> On 7/23/2015 5:55 PM, Alexander Scherbatiy wrote:
>>
>>  The test from the bug report does not use timer.stop().  Is it 
>> possible to write a test with timer.start()/stop() to reproduce the 
>> original issue?
>>
>>  Thanks,
>>  Alexandr.
>>
>> On 7/22/2015 11:56 AM, Semyon Sadetsky wrote:
>>> Yes, but then it will not very readable. I included you 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.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>




More information about the swing-dev mailing list