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

[Daniel Fuchs]

Hi Harsha,

  // We reached here because the executor was shutdown.
  // If executor was supplied by client, then it was shutdown
  // abruptly or JMXConnector was shutdown along with executor
  // while this thread was suspended at L564.
  if (!(executor instanceof LinearExecutor)) {
      // Spawn new executor that will do cleanup if JMXConnector is closed
      // or keep notif system running otherwise
      executor = new LinearExecutor();
....

I find it a bit strange to 'keep notif system running' if
the executor was shutdown.

If the user that supplied the executor shuts it down,
then does it make sense to continue running? Also
if the old executor has already been shutdown - and you
create a new one - then do we have any guarantee that
the new executor will be shut down properly?

I wonder whether you should instead perform the cleaning
action directly in the catch clause - that is - copy
the lines 553-562 over there, or maybe simply
call this::doRun() - without submitting 'this'
to an executor... But then we would have to have
some guarantee that doRun() will terminate and
not call doRun() again (hence my suggestion to
copy lines 553-562 instead)...

This code is very hairy - with states and multiple
critical sections and wait() and notifyAll(), so it
is difficult to reason about, and I can't guarantee
that it would be the right thing to do...
But maybe it would be worth prototyping it and see if
it also passes all the non-reg tests?

best regards,

-- daniel

On 11/11/16 16:02, Harsha Wardhana B wrote:
> Hello,
>
> Please review the fix for
>
> issue : https://bugs.openjdk.java.net/browse/JDK-8141591
>
> webrev : http://cr.openjdk.java.net/~hb/8141591/webrev.00/
>
> Fix details:
>
>> The root-cause for the issue is that NotifFetcher thread was suspended
>> at L562 after which main thread ran to completion shutting down both
>> client and server and stopping the executor. Fixing as
>>
>> synchronized (ClientNotifForwarder.this) {
>>          if (state == STARTED) {
>>             executor.execute(new NotifFetcher());
>>          }
>>       }
>>
>> will not solve the problem since NotifFetcher can get suspended after
>> checking state==STARTED and still hit the same problem.
>>
>> One way to solve would be to catch RejectedExecutionException and then
>> fallback on linearExecutor. This will take care of necessary cleanup
>> if client/server are shutdown or keeps the system running if executor
>> is abrupty shutdown by the client without stopping the client/server.
>
> The comments section of the issue also has discussion about various ways
> of fixing the issue and merits/demerits of different approaches. It also
> helps in getting a perspective on issue and the fix.
>
> Thanks
>
> Harsha
>