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

[Daniel Fuchs]

On 11/11/16 16:57, Harsha Wardhana B wrote:
> Hi Daniel,
> 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.

OK - the LinearExecutor has only one thread and that thread
will terminate gracefully when there is nothing more to
fetch - so shutdown of the new executor will not be an issue.

>> 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.

It's OK to continue in order to do clean up and
shutdown gracefully. It seems wrong to keep going afterwards
as if nothing had happened though (in the case the
user shuts the supplied executor down).

>> 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.

If executor is no longer final then it should be volatile.
And you should use double locking when resetting its value:

    if (!(executor  instanceof LinearExecutor)) {
        synchronized (ClientNotifForwarder.this) {
            if (!(executor  instanceof LinearExecutor)) {
                 executor = new LinearExecutor();
            }
        }
        executor.submit(this);
    }



-- daniel

>> 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