<AWT Dev> [12] Review Request: 8210231 Robot.delay() catches InterruptedException and prints stacktrace to stderr.

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

<AWT Dev> [12] Review Request: 8210231 Robot.delay() catches InterruptedException and prints stacktrace to stderr.

Sergey Bylokhov
Hello.
Please review the fix for jdk12.

Bug: https://bugs.openjdk.java.net/browse/JDK-8210231
Webrev: http://cr.openjdk.java.net/~serb/8210231/webrev.00

The Robot.delay() is a simple wrapper on top of Thread.sleep(), which ignores the InterruptedException. But in case of InterruptedException it prints the stack trace, which is unspecified behavior.

In the fix the "printStackTrace()" was removed, and the interrupted state of the thread is restored.

--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [12] Review Request: 8210231 Robot.delay() catches InterruptedException and prints stacktrace to stderr.

Krishna Addepalli
+1

Thanks,
Krishna

-----Original Message-----
From: Sergey Bylokhov
Sent: Monday, October 8, 2018 10:26 AM
To: [hidden email]
Subject: <AWT Dev> [12] Review Request: 8210231 Robot.delay() catches InterruptedException and prints stacktrace to stderr.

Hello.
Please review the fix for jdk12.

Bug: https://bugs.openjdk.java.net/browse/JDK-8210231
Webrev: http://cr.openjdk.java.net/~serb/8210231/webrev.00

The Robot.delay() is a simple wrapper on top of Thread.sleep(), which ignores the InterruptedException. But in case of InterruptedException it prints the stack trace, which is unspecified behavior.

In the fix the "printStackTrace()" was removed, and the interrupted state of the thread is restored.

--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [12] Review Request: 8210231 Robot.delay() catches InterruptedException and prints stacktrace to stderr.

Sergey Bylokhov
Hi, Krishna.
Thank you for review! Any volunteers for the second +1? =)

On 07/10/2018 22:02, Krishna Addepalli wrote:

> +1
>
> Thanks,
> Krishna
>
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Monday, October 8, 2018 10:26 AM
> To: [hidden email]
> Subject: <AWT Dev> [12] Review Request: 8210231 Robot.delay() catches InterruptedException and prints stacktrace to stderr.
>
> Hello.
> Please review the fix for jdk12.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210231
> Webrev: http://cr.openjdk.java.net/~serb/8210231/webrev.00
>
> The Robot.delay() is a simple wrapper on top of Thread.sleep(), which ignores the InterruptedException. But in case of InterruptedException it prints the stack trace, which is unspecified behavior.
>
> In the fix the "printStackTrace()" was removed, and the interrupted state of the thread is restored.
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [12] Review Request: 8210231 Robot.delay() catches InterruptedException and prints stacktrace to stderr.

Shashidhara H V
Hi Sergey, The changes looks good to me.

Thanks and regards,
Shashi

-----Original Message-----
From: Sergey Bylokhov
Sent: Tuesday, October 30, 2018 6:28 AM
To: Krishna Addepalli <[hidden email]>; [hidden email]
Subject: Re: <AWT Dev> [12] Review Request: 8210231 Robot.delay() catches InterruptedException and prints stacktrace to stderr.

Hi, Krishna.
Thank you for review! Any volunteers for the second +1? =)

On 07/10/2018 22:02, Krishna Addepalli wrote:

> +1
>
> Thanks,
> Krishna
>
> -----Original Message-----
> From: Sergey Bylokhov
> Sent: Monday, October 8, 2018 10:26 AM
> To: [hidden email]
> Subject: <AWT Dev> [12] Review Request: 8210231 Robot.delay() catches InterruptedException and prints stacktrace to stderr.
>
> Hello.
> Please review the fix for jdk12.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210231
> Webrev: http://cr.openjdk.java.net/~serb/8210231/webrev.00
>
> The Robot.delay() is a simple wrapper on top of Thread.sleep(), which ignores the InterruptedException. But in case of InterruptedException it prints the stack trace, which is unspecified behavior.
>
> In the fix the "printStackTrace()" was removed, and the interrupted state of the thread is restored.
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [12] Review Request: 8210231 Robot.delay() catches InterruptedException and prints stacktrace to stderr.

Phil Race
In reply to this post by Sergey Bylokhov
 > and the interrupted state of the thread is restored

Can you explain why you are doing this ?
Thread.sleep() clears the IE so why do you need to restore it ?
It just looks like an un-specified and un-needed change of behaviour to me.

-phil.


On 10/7/18 9:56 PM, Sergey Bylokhov wrote:

> Hello.
> Please review the fix for jdk12.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210231
> Webrev: http://cr.openjdk.java.net/~serb/8210231/webrev.00
>
> The Robot.delay() is a simple wrapper on top of Thread.sleep(), which
> ignores the InterruptedException. But in case of InterruptedException
> it prints the stack trace, which is unspecified behavior.
>
> In the fix the "printStackTrace()" was removed, and the interrupted
> state of the thread is restored.
>

Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [12] Review Request: 8210231 Robot.delay() catches InterruptedException and prints stacktrace to stderr.

Sergey Bylokhov
Hi, Phil.

On 30/10/2018 10:34, Phil Race wrote:
> Can you explain why you are doing this ?
> Thread.sleep() clears the IE so why do you need to restore it ?
> It just looks like an un-specified and un-needed change of behaviour to me.

It is done to "notify" other code which follows Robot.delay() that the thread was interrupted.

So for example before the fix the code below depended on when the thread.interrupt() was called:
   Robot.delay(10);
   EventQueue.invokeAndWait(...);

If the method "interrupt()" was called by the application, when invokeAndWait () was called, then the thread will be stopped as expected.
But if the method "interrupt()" is called during Robot.delay(10), the application will miss the fact that the flow was interrupted.

>
> -phil.
>
>
> On 10/7/18 9:56 PM, Sergey Bylokhov wrote:
>> Hello.
>> Please review the fix for jdk12.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8210231
>> Webrev: http://cr.openjdk.java.net/~serb/8210231/webrev.00
>>
>> The Robot.delay() is a simple wrapper on top of Thread.sleep(), which ignores the InterruptedException. But in case of InterruptedException it prints the stack trace, which is unspecified behavior.
>>
>> In the fix the "printStackTrace()" was removed, and the interrupted state of the thread is restored.
>>
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [12] Review Request: 8210231 Robot.delay() catches InterruptedException and prints stacktrace to stderr.

Phil Race
But the docs for Robot.delay() state :

 >  To catch any InterruptedExceptions that occur, Thread.sleep() may be used instead.

So to my reading it is telling you "this call will completely swallow thread interrupts".

And you are changing that and I don't think you should.
It has nothing to do with removing the printStackTrace which is the only
change requested by the submitter.

-phil.

On 10/30/18 11:04 AM, Sergey Bylokhov wrote:
Hi, Phil.

On 30/10/2018 10:34, Phil Race wrote:
Can you explain why you are doing this ?
Thread.sleep() clears the IE so why do you need to restore it ?
It just looks like an un-specified and un-needed change of behaviour to me.

It is done to "notify" other code which follows Robot.delay() that the thread was interrupted.

So for example before the fix the code below depended on when the thread.interrupt() was called:
  Robot.delay(10);
  EventQueue.invokeAndWait(...);

If the method "interrupt()" was called by the application, when invokeAndWait () was called, then the thread will be stopped as expected.
But if the method "interrupt()" is called during Robot.delay(10), the application will miss the fact that the flow was interrupted.


-phil.


On 10/7/18 9:56 PM, Sergey Bylokhov wrote:
Hello.
Please review the fix for jdk12.

Bug: https://bugs.openjdk.java.net/browse/JDK-8210231
Webrev: http://cr.openjdk.java.net/~serb/8210231/webrev.00

The Robot.delay() is a simple wrapper on top of Thread.sleep(), which ignores the InterruptedException. But in case of InterruptedException it prints the stack trace, which is unspecified behavior.

In the fix the "printStackTrace()" was removed, and the interrupted state of the thread is restored.





Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [12] Review Request: 8210231 Robot.delay() catches InterruptedException and prints stacktrace to stderr.

Sergey Bylokhov
On 30/10/2018 14:03, Phil Race wrote:
> But the docs for Robot.delay() state :
>
>   >  To catch any |InterruptedException|s that occur, |Thread.sleep()| may be used instead.
>
> So to my reading it is telling you "this call will completely swallow thread interrupts".

This is what I have tried to follow, if the user need to catch the InterruptedException then Thread.sleep() should be used. Otherwise the Robot.delay() may be used, which will be transparent to the Thread.interrupt(). I mean behavior will not be depend on the moment when interrupt was called, before Robot.delay(), inside Robot.delay() or right after the Robot.delay().

>
> And you are changing that and I don't think you should.
> It has nothing to do with removing the printStackTrace which is the only
> change requested by the submitter.
>
> -phil.
>
> On 10/30/18 11:04 AM, Sergey Bylokhov wrote:
>> Hi, Phil.
>>
>> On 30/10/2018 10:34, Phil Race wrote:
>>> Can you explain why you are doing this ?
>>> Thread.sleep() clears the IE so why do you need to restore it ?
>>> It just looks like an un-specified and un-needed change of behaviour to me.
>>
>> It is done to "notify" other code which follows Robot.delay() that the thread was interrupted.
>>
>> So for example before the fix the code below depended on when the thread.interrupt() was called:
>>   Robot.delay(10);
>>   EventQueue.invokeAndWait(...);
>>
>> If the method "interrupt()" was called by the application, when invokeAndWait () was called, then the thread will be stopped as expected.
>> But if the method "interrupt()" is called during Robot.delay(10), the application will miss the fact that the flow was interrupted.
>>
>>>
>>> -phil.
>>>
>>>
>>> On 10/7/18 9:56 PM, Sergey Bylokhov wrote:
>>>> Hello.
>>>> Please review the fix for jdk12.
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8210231
>>>> Webrev: http://cr.openjdk.java.net/~serb/8210231/webrev.00
>>>>
>>>> The Robot.delay() is a simple wrapper on top of Thread.sleep(), which ignores the InterruptedException. But in case of InterruptedException it prints the stack trace, which is unspecified behavior.
>>>>
>>>> In the fix the "printStackTrace()" was removed, and the interrupted state of the thread is restored.
>>>>
>>>
>>
>>
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [12] Review Request: 8210231 Robot.delay() catches InterruptedException and prints stacktrace to stderr.

Phil Race
But Robot.delay() is inherently a long lived operation, so interrupts are
more likely there. And you *are* changing behaviour. So I am still
unconvinced this is right. When might this matter in practice in writing
tests ?

-phil.

On 10/30/18, 3:17 PM, Sergey Bylokhov wrote:

> On 30/10/2018 14:03, Phil Race wrote:
>> But the docs for Robot.delay() state :
>>
>> >  To catch any |InterruptedException|s that occur, |Thread.sleep()|
>> may be used instead.
>>
>> So to my reading it is telling you "this call will completely swallow
>> thread interrupts".
>
> This is what I have tried to follow, if the user need to catch the
> InterruptedException then Thread.sleep() should be used. Otherwise the
> Robot.delay() may be used, which will be transparent to the
> Thread.interrupt(). I mean behavior will not be depend on the moment
> when interrupt was called, before Robot.delay(), inside Robot.delay()
> or right after the Robot.delay().
>
>>
>> And you are changing that and I don't think you should.
>> It has nothing to do with removing the printStackTrace which is the only
>> change requested by the submitter.
>>
>> -phil.
>>
>> On 10/30/18 11:04 AM, Sergey Bylokhov wrote:
>>> Hi, Phil.
>>>
>>> On 30/10/2018 10:34, Phil Race wrote:
>>>> Can you explain why you are doing this ?
>>>> Thread.sleep() clears the IE so why do you need to restore it ?
>>>> It just looks like an un-specified and un-needed change of
>>>> behaviour to me.
>>>
>>> It is done to "notify" other code which follows Robot.delay() that
>>> the thread was interrupted.
>>>
>>> So for example before the fix the code below depended on when the
>>> thread.interrupt() was called:
>>>   Robot.delay(10);
>>>   EventQueue.invokeAndWait(...);
>>>
>>> If the method "interrupt()" was called by the application, when
>>> invokeAndWait () was called, then the thread will be stopped as
>>> expected.
>>> But if the method "interrupt()" is called during Robot.delay(10),
>>> the application will miss the fact that the flow was interrupted.
>>>
>>>>
>>>> -phil.
>>>>
>>>>
>>>> On 10/7/18 9:56 PM, Sergey Bylokhov wrote:
>>>>> Hello.
>>>>> Please review the fix for jdk12.
>>>>>
>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8210231
>>>>> Webrev: http://cr.openjdk.java.net/~serb/8210231/webrev.00
>>>>>
>>>>> The Robot.delay() is a simple wrapper on top of Thread.sleep(),
>>>>> which ignores the InterruptedException. But in case of
>>>>> InterruptedException it prints the stack trace, which is
>>>>> unspecified behavior.
>>>>>
>>>>> In the fix the "printStackTrace()" was removed, and the
>>>>> interrupted state of the thread is restored.
>>>>>
>>>>
>>>
>>>
>>
>
>
Reply | Threaded
Open this post in threaded view
|

Re: <AWT Dev> [12] Review Request: 8210231 Robot.delay() catches InterruptedException and prints stacktrace to stderr.

Sergey Bylokhov
On 30/10/2018 15:52, Philip Race wrote:
> But Robot.delay() is inherently a long lived operation, so interrupts are
> more likely there. And you *are* changing behaviour. So I am still
> unconvinced this is right. When might this matter in practice in writing tests ?

This might matter for example if the next sequence of code is interrupted by the test harness:
Robot.delay(10);
EventQueue.invokeAndWait(...); or any other code which may throw "InterruptedException".

Depending on when the interrupt will happen the test will continue or will stop, the fix will removed this dependency.

>
> -phil.
>
> On 10/30/18, 3:17 PM, Sergey Bylokhov wrote:
>> On 30/10/2018 14:03, Phil Race wrote:
>>> But the docs for Robot.delay() state :
>>>
>>> >  To catch any |InterruptedException|s that occur, |Thread.sleep()| may be used instead.
>>>
>>> So to my reading it is telling you "this call will completely swallow thread interrupts".
>>
>> This is what I have tried to follow, if the user need to catch the InterruptedException then Thread.sleep() should be used. Otherwise the Robot.delay() may be used, which will be transparent to the Thread.interrupt(). I mean behavior will not be depend on the moment when interrupt was called, before Robot.delay(), inside Robot.delay() or right after the Robot.delay().
>>
>>>
>>> And you are changing that and I don't think you should.
>>> It has nothing to do with removing the printStackTrace which is the only
>>> change requested by the submitter.
>>>
>>> -phil.
>>>
>>> On 10/30/18 11:04 AM, Sergey Bylokhov wrote:
>>>> Hi, Phil.
>>>>
>>>> On 30/10/2018 10:34, Phil Race wrote:
>>>>> Can you explain why you are doing this ?
>>>>> Thread.sleep() clears the IE so why do you need to restore it ?
>>>>> It just looks like an un-specified and un-needed change of behaviour to me.
>>>>
>>>> It is done to "notify" other code which follows Robot.delay() that the thread was interrupted.
>>>>
>>>> So for example before the fix the code below depended on when the thread.interrupt() was called:
>>>>   Robot.delay(10);
>>>>   EventQueue.invokeAndWait(...);
>>>>
>>>> If the method "interrupt()" was called by the application, when invokeAndWait () was called, then the thread will be stopped as expected.
>>>> But if the method "interrupt()" is called during Robot.delay(10), the application will miss the fact that the flow was interrupted.
>>>>
>>>>>
>>>>> -phil.
>>>>>
>>>>>
>>>>> On 10/7/18 9:56 PM, Sergey Bylokhov wrote:
>>>>>> Hello.
>>>>>> Please review the fix for jdk12.
>>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8210231
>>>>>> Webrev: http://cr.openjdk.java.net/~serb/8210231/webrev.00
>>>>>>
>>>>>> The Robot.delay() is a simple wrapper on top of Thread.sleep(), which ignores the InterruptedException. But in case of InterruptedException it prints the stack trace, which is unspecified behavior.
>>>>>>
>>>>>> In the fix the "printStackTrace()" was removed, and the interrupted state of the thread is restored.
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>


--
Best regards, Sergey.