jmx-dev RFR 8141591: javax/management/remote/mandatory/threads/ExecutorTest.java fails intermittently

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

jmx-dev RFR 8141591: javax/management/remote/mandatory/threads/ExecutorTest.java fails intermittently

Jaroslav Bachorik
Please, review the following test change

Issue : https://bugs.openjdk.java.net/browse/JDK-8141591
Webrev: http://cr.openjdk.java.net/~jbachorik/8141591/webrev.00

In rare circumstances, when an external executor is provided, the
ClientNotifForwarder$NotifFetcher.doRun() method might fail because of
RejectedExecutionException caused by the executor being externally shut
down.

The patch adds a guard against this possibility. If the executor has
been shut down the fetcher will also properly stop.

Thanks,

-JB-
Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8141591: javax/management/remote/mandatory/threads/ExecutorTest.java fails intermittently

Shanliang JIANG
Hi Jaroslav,

The issue is that after a JMX client is terminated, its ClientNotifForwarder continues deliver a job to a user specific Executor, I think a better fix to not allow this happen. 

I am not sure that it is a good solution to check RejectedExecutionException, here is the Java doc:
"Exception thrown by an Executor when a task cannot be accepted for execution."

it means that  the exception is possibly thrown in other cases too, like too many tasks if it is shared. So ignore simply this exception in case of not “shouldStop()” seems incorrect.

And Executor is an interface and a user could provide any implementation class, so possible a user would throw any another RuntimeException or even an Error in this case.

Shanliang
On 12 Nov 2015, at 13:13, Jaroslav Bachorik <[hidden email]> wrote:

Please, review the following test change

Issue : https://bugs.openjdk.java.net/browse/JDK-8141591
Webrev: http://cr.openjdk.java.net/~jbachorik/8141591/webrev.00

In rare circumstances, when an external executor is provided, the ClientNotifForwarder$NotifFetcher.doRun() method might fail because of RejectedExecutionException caused by the executor being externally shut down.

The patch adds a guard against this possibility. If the executor has been shut down the fetcher will also properly stop.

Thanks,

-JB-

Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8141591: javax/management/remote/mandatory/threads/ExecutorTest.java fails intermittently

Jaroslav Bachorik
On 13.11.2015 08:05, Shanliang JIANG wrote:

> Hi Jaroslav,
>
> The issue is that after a JMX client is terminated, its
> ClientNotifForwarder continues deliver a job to a user specific
> Executor, I think a better fix to not allow this happen.
>
> I am not sure that it is a good solution to check
> RejectedExecutionException, here is the Java doc:
> "Exception thrown by an |Executor|
> <file:///Users/sjiang/projects/jdk/workspace/fix/javadoc9/java/util/concurrent/Executor.html> when
> a task cannot be accepted for execution."
>
> it means that  the exception is possibly thrown in other cases too, like
> too many tasks if it is shared. So ignore simply this exception in case
> of not “shouldStop()” seems incorrect.
>
> And Executor is an interface and a user could provide any implementation
> class, so possible a user would throw any another RuntimeException or
> even an Error in this case.
Well, the main problem is the self-rescheduling part. Normally, a scheduled executor would be used to perform periodic tasks and it would know how to handle its own shutdown. But, unfortunately, the more generic Executor is required. If only it were at least ExecutorService where you can use 'isShutdown()' method ...

The fact that the user provided executor can throw RejectedExecutionException at its discretion opens whole another can of worms. The code as it is now will simply bail out of the notification fetching loop with the thrown exception. Sadly, there is no cleanup performed so the ClientNotifForwarder will stay in inconsistent state (marked as STARTED when it is, in fact, TERMINATED, notification listeners not de-registered, etc.). To make things worse there seem to be no official documentation as what is the expected behaviour of the externally provided executor. The only documentation to the env property "jmx.remote.x.fetch.notifications.executor" is in ClientNotifForwarder.java (L125-128) and it is not exactly exhaustive.

-JB-


>
> Shanliang
> > On 12 Nov 2015, at 13:13, Jaroslav Bachorik
> > <[hidden email] <mailto:[hidden email]>>
> > wrote:
> >
> > Please, review the following test change
> >
> > Issue : https://bugs.openjdk.java.net/browse/JDK-8141591
> > Webrev: http://cr.openjdk.java.net/~jbachorik/8141591/webrev.00
> >
> > In rare circumstances, when an external executor is provided, the
> > ClientNotifForwarder$NotifFetcher.doRun() method might fail because of
> > RejectedExecutionException caused by the executor being externally
> > shut down.
> >
> > The patch adds a guard against this possibility. If the executor has
> > been shut down the fetcher will also properly stop.
> >
> > Thanks,
> >
> > -JB-
>

Reply | Threaded
Open this post in threaded view
|

Re: jmx-dev RFR 8141591: javax/management/remote/mandatory/threads/ExecutorTest.java fails intermittently

Shanliang JIANG

On 13 Nov 2015, at 09:04, Jaroslav Bachorik <[hidden email]> wrote:

On 13.11.2015 08:05, Shanliang JIANG wrote:
Hi Jaroslav,

The issue is that after a JMX client is terminated, its
ClientNotifForwarder continues deliver a job to a user specific
Executor, I think a better fix to not allow this happen.

I am not sure that it is a good solution to check
RejectedExecutionException, here is the Java doc:
"Exception thrown by an |Executor|
<file:///Users/sjiang/projects/jdk/workspace/fix/javadoc9/java/util/concurrent/Executor.html> when
a task cannot be accepted for execution."

it means that  the exception is possibly thrown in other cases too, like
too many tasks if it is shared. So ignore simply this exception in case
of not “shouldStop()” seems incorrect.

And Executor is an interface and a user could provide any implementation
class, so possible a user would throw any another RuntimeException or
even an Error in this case.
Well, the main problem is the self-rescheduling part. Normally, a scheduled executor would be used to perform periodic tasks and it would know how to handle its own shutdown. But, unfortunately, the more generic Executor is required. If only it were at least ExecutorService where you can use 'isShutdown()' method ...

The fact that the user provided executor can throw RejectedExecutionException at its discretion opens whole another can of worms. The code as it is now will simply bail out of the notification fetching loop with the thrown exception. Sadly, there is no cleanup performed so the ClientNotifForwarder will stay in inconsistent state (marked as STARTED when it is, in fact, TERMINATED, notification listeners not de-registered, etc.). To make things worse there seem to be no official documentation as what is the expected behaviour of the externally provided executor. The only documentation to the env property "jmx.remote.x.fetch.notifications.executor" is in ClientNotifForwarder.java (L125-128) and it is not exactly exhaustive.

Here is my suggestion (I create a webrev because it is easier to look at):

It does rescheduling within the synchronisation, which makes sure to not deliver a new task after a JMX client is terminated. 

This class is complicated because the fetching should be stopped if no more listener or during re-connection, but then could be re-started at anytime.

Shanliang



-JB-



Shanliang

> On 12 Nov 2015, at 13:13, Jaroslav Bachorik
> <[hidden email] <[hidden email]>>
> wrote:
>
> Please, review the following test change
>
> Issue : https://bugs.openjdk.java.net/browse/JDK-8141591
> Webrev: http://cr.openjdk.java.net/~jbachorik/8141591/webrev.00
>
> In rare circumstances, when an external executor is provided, the
> ClientNotifForwarder$NotifFetcher.doRun() method might fail because of
> RejectedExecutionException caused by the executor being externally
> shut down.
>
> The patch adds a guard against this possibility. If the executor has
> been shut down the fetcher will also properly stop.
>
> Thanks,
>
> -JB-