<Swing Dev> [9] Review Request for 8130735: javax.swing.TimerQueue: timer fires late when another timer starts
Semyon Sadetsky
semyon.sadetsky at oracle.com
Thu Jul 23 15:26:37 UTC 2015
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