[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: jmx-dev RFR 8141591: javax/management/remote/mandatory/threads/ExecutorTest.java
From: Shanliang JIANG <shanliang.jiang () oracle ! com>
Date: 2015-11-13 17:07:42
Message-ID: 0838E637-4F57-4BED-B973-3AAACABC719C () oracle ! com
[Download RAW message or body]
> On 13 Nov 2015, at 09:04, Jaroslav Bachorik <jaroslav.bachorik@oracle.com> 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
> > > <jaroslav.bachorik@oracle.com <mailto:jaroslav.bachorik@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-
> >
>
[Attachment #3 (unknown)]
<html><head><meta http-equiv="Content-Type" content="text/html \
charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; \
-webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote \
type="cite" class=""><div class="">On 13 Nov 2015, at 09:04, Jaroslav Bachorik <<a \
href="mailto:jaroslav.bachorik@oracle.com" \
class="">jaroslav.bachorik@oracle.com</a>> wrote:</div><br \
class="Apple-interchange-newline"><div class="">On 13.11.2015 08:05, Shanliang JIANG \
wrote:<br class=""><blockquote type="cite" class="">Hi Jaroslav,<br class=""><br \
class="">The issue is that after a JMX client is terminated, its<br \
class="">ClientNotifForwarder continues deliver a job to a user specific<br \
class="">Executor, I think a better fix to not allow this happen.<br class=""><br \
class="">I am not sure that it is a good solution to check<br \
class="">RejectedExecutionException, here is the Java doc:<br class="">"Exception \
thrown by an |Executor|<br class=""><<a \
href="file:///Users/sjiang/projects/jdk/workspace/fix/javadoc9/java/util/concurrent/Executor.html" \
class="">file:///Users/sjiang/projects/jdk/workspace/fix/javadoc9/java/util/concurrent/Executor.html</a>> \
when<br class="">a task cannot be accepted for execution."<br class=""><br \
class="">it means that the exception is possibly thrown in other cases too, \
like<br class="">too many tasks if it is shared. So ignore simply this exception in \
case<br class="">of not "shouldStop()" seems incorrect.<br class=""><br class="">And \
Executor is an interface and a user could provide any implementation<br \
class="">class, so possible a user would throw any another RuntimeException or<br \
class="">even an Error in this case.<br class=""></blockquote>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 ...<br class=""><br \
class="">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.<br class=""></div></blockquote><div><br \
class=""></div>Here is my suggestion (I create a webrev because it is easier to look \
at):</div><div><span class="Apple-tab-span" style="white-space:pre"> </span><a \
href="http://cr.openjdk.java.net/~sjiang/JDK-8141591/00/" \
class="">http://cr.openjdk.java.net/~sjiang/JDK-8141591/00/</a></div><div><br \
class=""></div><div>It does rescheduling within the synchronisation, which makes sure \
to not deliver a new task after a JMX client is terminated. </div><div><br \
class=""></div><div>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.</div><div><br class=""></div><div>Shanliang</div><div><br \
class=""></div><div><br class=""><blockquote type="cite" class=""><div class=""><br \
class="">-JB-<br class=""><br class=""><br class=""><blockquote type="cite" \
class=""><br class="">Shanliang<br class="">> On 12 Nov 2015, at 13:13, Jaroslav \
Bachorik<br class="">> <<a href="mailto:jaroslav.bachorik@oracle.com" \
class="">jaroslav.bachorik@oracle.com</a> <<a \
href="mailto:jaroslav.bachorik@oracle.com" \
class="">mailto:jaroslav.bachorik@oracle.com</a>>><br class="">> wrote:<br \
class="">><br class="">> Please, review the following test change<br \
class="">><br class="">> Issue : <a \
href="https://bugs.openjdk.java.net/browse/JDK-8141591" \
class="">https://bugs.openjdk.java.net/browse/JDK-8141591</a><br class="">> \
Webrev: <a href="http://cr.openjdk.java.net/~jbachorik/8141591/webrev.00" \
class="">http://cr.openjdk.java.net/~jbachorik/8141591/webrev.00</a><br \
class="">><br class="">> In rare circumstances, when an external executor is \
provided, the<br class="">> ClientNotifForwarder$NotifFetcher.doRun() method might \
fail because of<br class="">> RejectedExecutionException caused by the executor \
being externally<br class="">> shut down.<br class="">><br class="">> The \
patch adds a guard against this possibility. If the executor has<br class="">> \
been shut down the fetcher will also properly stop.<br class="">><br class="">> \
Thanks,<br class="">><br class="">> -JB-<br class=""><br \
class=""></blockquote><br class=""></div></blockquote></div><br \
class=""></body></html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic