<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