This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
From javadoc of shutdown() method: Initiates an orderly shutdown in which *previously submitted tasks are executed*, but no new tasks will be accepted. Invocation has no additional effect if already shut down. The shutdown() as implemented in RequestProcessor won't execute any previously submitted tasks. I would also expect that shutdown() call won't interrupt running tasks as shutdownNow() is supposed to do that.
Created attachment 102867 [details] patch Attached patch contains tests for both problems. It also contains a simple fix. The fix should be reviewed by somebody more experienced in this area.
If I understand it correctly (just looking at the patch), it looks reasonable. Threading behavior tests are really hard to make reliable (sometimes perfection is impossible). testSubmittedTasksExecutedBeforeShutdown is one such - you can only prove that it hasn't been run before the test method exits, not that it never will be :-) And theoretically, on Linux at least, the thread could be interrupted because a signal was sent to the process, so it's impossible to prove that the thread was interrupted for the right reason. But we can only do so much, and it's better to have tests than not :-/ TB01: Why submit the runnable twice? It's good to test that the behavior is the same for multiple runnables and a single runnable, but I'd suggest having two tests for that. TB02: May not be terribly useful to print stack traces on InterruptedExceptions in the test method body (I may have done this for debugging purposes in other tests, not sure). TB03: One thing that might be worth considering is whether the behavior on shutdown should be different for Cancellables - that's outside the scope of ExecutorService, since that has no concept for such things. IIRC, ExecutorService does, however, have shutdownNow(), which returns all the pending Runnables (the caller that shuts it down can then run them if it wants) - which accomplishes the same thing if the caller knows what they are and can do something with them. I'm not sure if it would be the right behavior to call cancel() on any pending cancellables, but if you don't do that, it's probably a good idea to mention in the javadoc for our implementation of shutdownNow() that if it's important that things which will not be run get cancelled, the caller should cancel them (use case where I've needed this behavior: having a queue of, for example, messages to send asynchronously which I need to save on VM shutdown if not sent).
Why was this assigned to phejl; moreover, without any comment? The owner of RP should be primarily responsible for this.
Probably because the patch was written by Petr, reviewed by Tim and it can be integrated by Petr whenever he feels it shall be integrated.
Fixed in web-main f44518d8b5c7. (In reply to comment #2) > If I understand it correctly (just looking at the patch), it looks reasonable. > > Threading behavior tests are really hard to make reliable (sometimes perfection > is impossible). > > testSubmittedTasksExecutedBeforeShutdown is one such - you can only prove that > it hasn't been run before the test method exits, not that it never will be :-) > And theoretically, on Linux at least, the thread could be interrupted because a > signal was sent to the process, so it's impossible to prove that the thread was > interrupted for the right reason. But we can only do so much, and it's better > to have tests than not :-/ I agree. I guess we can only mark it RandomlyFails in case this would cause a problems. > > TB01: Why submit the runnable twice? It's good to test that the behavior is > the same for multiple runnables and a single runnable, but I'd suggest having > two tests for that. The first one blocks the RP. The second is thus guaranteed to be waiting in queue - we want to test that the second is executed as well. > > TB02: May not be terribly useful to print stack traces on InterruptedExceptions > in the test method body (I may have done this for debugging purposes in other > tests, not sure). I agree. Test methods are throwing it now. > > TB03: One thing that might be worth considering is whether the behavior on > shutdown should be different for Cancellables - that's outside the scope of > ExecutorService, since that has no concept for such things. I think this is other enhancement - and there is another bug in shutdownNow(). I'll file it. > > IIRC, ExecutorService does, however, have shutdownNow(), which returns all the > pending Runnables (the caller that shuts it down can then run them if it wants) > - which accomplishes the same thing if the caller knows what they are and can > do something with them. Thats the bug - it returns awaiting tasks (not the running tasks) afaik. > > I'm not sure if it would be the right behavior to call cancel() on any pending > cancellables, but if you don't do that, it's probably a good idea to mention in > the javadoc for our implementation of shutdownNow() that if it's important that > things which will not be run get cancelled, the caller should cancel them (use > case where I've needed this behavior: having a queue of, for example, messages > to send asynchronously which I need to save on VM shutdown if not sent). Fixed in web-main f44518d8b5c7.
(In reply to comment #5) > > TB03: One thing that might be worth considering is whether the behavior on > > shutdown should be different for Cancellables - that's outside the scope of > > ExecutorService, since that has no concept for such things. > I think this is other enhancement - and there is another bug in shutdownNow(). > I'll file it. Issue 191846 and 191848.