<Swing Dev> [9] Review Request for 8130735: javax.swing.TimerQueue: timer fires late when another timer starts
Sergey Bylokhov
Sergey.Bylokhov at oracle.com
Fri Jul 24 15:38:25 UTC 2015
Looks fine to me too. Thanks.
On 24.07.15 12:45, Alexander Scherbatiy wrote:
>
> 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.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
--
Best regards, Sergey.
More information about the swing-dev
mailing list