Re: RFR : JDK-8141591 - javax/management/remote/mandatory/threads/ExecutorTest.java fails intermittently
Hi Harsha, I'm still unsure that creating a new executor is the right solution, but we can give it a try... good to go. best regards, -- daniel On 17/11/16 13:14, Harsha Wardhana B wrote: Hello, Gentle reminder !! -Harsha On Tuesday 15 November 2016 02:16 PM, Harsha Wardhana B wrote: Hello, Please review below webrev incorporating Daniel's comments. http://cr.openjdk.java.net/~hb/8141591/webrev.01/ Regards Harsha On Monday 14 November 2016 04:14 PM, Daniel Fuchs wrote: On 14/11/16 06:57, Harsha Wardhana B wrote: 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). With current changes, we do continue to carry on with linear executor. If the user wants to shutdown the system, he can always do it by shutting down client and server along with executor. If he shuts down executor but not client/server, it may be safe to assume that he wants the system to be up and running. It may not be appropriate to assume user wants to shutdown notif system just because he shutdown executor. Well, it may also not be appropriate to invoke a user provided callback on another executor than the one provided by the user. If the user provides an executor, we must assume he has good reasons to do so. I'm not hard set to opposing to what you propose, but it makes me feel uncomfortable. best regards, -- daniel
Re: RFR : JDK-8141591 - javax/management/remote/mandatory/threads/ExecutorTest.java fails intermittently
Hello, Please review below webrev incorporating Daniel's comments. http://cr.openjdk.java.net/~hb/8141591/webrev.01/ Regards Harsha On Monday 14 November 2016 04:14 PM, Daniel Fuchs wrote: On 14/11/16 06:57, Harsha Wardhana B wrote: 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). With current changes, we do continue to carry on with linear executor. If the user wants to shutdown the system, he can always do it by shutting down client and server along with executor. If he shuts down executor but not client/server, it may be safe to assume that he wants the system to be up and running. It may not be appropriate to assume user wants to shutdown notif system just because he shutdown executor. Well, it may also not be appropriate to invoke a user provided callback on another executor than the one provided by the user. If the user provides an executor, we must assume he has good reasons to do so. I'm not hard set to opposing to what you propose, but it makes me feel uncomfortable. best regards, -- daniel
Re: RFR : JDK-8141591 - javax/management/remote/mandatory/threads/ExecutorTest.java fails intermittently
On 14/11/16 06:57, Harsha Wardhana B wrote: 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). With current changes, we do continue to carry on with linear executor. If the user wants to shutdown the system, he can always do it by shutting down client and server along with executor. If he shuts down executor but not client/server, it may be safe to assume that he wants the system to be up and running. It may not be appropriate to assume user wants to shutdown notif system just because he shutdown executor. Well, it may also not be appropriate to invoke a user provided callback on another executor than the one provided by the user. If the user provides an executor, we must assume he has good reasons to do so. I'm not hard set to opposing to what you propose, but it makes me feel uncomfortable. best regards, -- daniel
Re: RFR : JDK-8141591 - javax/management/remote/mandatory/threads/ExecutorTest.java fails intermittently
Hi Daniel, On Friday 11 November 2016 11:02 PM, Daniel Fuchs wrote: 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). With current changes, we do continue to carry on with linear executor. If the user wants to shutdown the system, he can always do it by shutting down client and server along with executor. If he shuts down executor but not client/server, it may be safe to assume that he wants the system to be up and running. It may not be appropriate to assume user wants to shutdown notif system just because he shutdown executor. 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); } Sure. Will send the updated webrev. -- 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 -Harsha
Re: RFR : JDK-8141591 - javax/management/remote/mandatory/threads/ExecutorTest.java fails intermittently
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
Re: RFR : JDK-8141591 - javax/management/remote/mandatory/threads/ExecutorTest.java fails intermittently
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
Re: RFR : JDK-8141591 - javax/management/remote/mandatory/threads/ExecutorTest.java fails intermittently
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
RFR : JDK-8141591 - javax/management/remote/mandatory/threads/ExecutorTest.java fails intermittently
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