> On 13 Nov 2015, at 09:04, Jaroslav Bachorik <[email protected]>
> 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):
http://cr.openjdk.java.net/~sjiang/JDK-8141591/00/
<http://cr.openjdk.java.net/~sjiang/JDK-8141591/00/>
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
>> > <[email protected] <mailto:[email protected]>>
>> > 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-
>>
>
