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
> > <jaroslav.bachorik at oracle.com <mailto:jaroslav.bachorik at oracle.com>>
> > 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-
>