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

[Harsha Wardhana B]

Hi Daniel,


On Friday 11 November 2016 10:12 PM, Daniel Fuchs wrote:
> 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.
>
It may happen that user will not be aware of shutdown sequence 
(client->server->executor) and may shutdown executor first. In that 
case, we may need to keep notif system running.
> 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?
Old executor will be shutdown by the user. We are only swapping a user 
supplied executor with new linear executor. It will follow the same 
shutdown sequence as original one.
>
> 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)...
Well, that will not cover the case where user shuts down executor but 
keeps the client/server alive. So we will need a executor that can keep 
notif system running as well as do clean-up if client/server is closed.
>
> 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?
>
Yes Daniel. It is a precarious section of code loosely held together by 
states. However, I think the changes are not too intrusive since it does 
not rely on states. It only looks for RejectedExecutionException. I have 
written test-case that can validate my changes. I haven't JPRT yet but 
would be happy to share test results if required.
> 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
>>
>
Thanks
Harsha