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.

Bug 191767 - RequestProcessor violates API contract of ExecutorService
Summary: RequestProcessor violates API contract of ExecutorService
Status: RESOLVED FIXED
Alias: None
Product: platform
Classification: Unclassified
Component: -- Other -- (show other bugs)
Version: 7.0
Hardware: PC All
: P2 normal (vote)
Assignee: Petr Hejl
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-09 20:46 UTC by Petr Hejl
Modified: 2010-11-11 13:42 UTC (History)
5 users (show)

See Also:
Issue Type: DEFECT
Exception Reporter:


Attachments
patch (4.49 KB, patch)
2010-11-09 20:50 UTC, Petr Hejl
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Petr Hejl 2010-11-09 20:46:42 UTC
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.
Comment 1 Petr Hejl 2010-11-09 20:50:45 UTC
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.
Comment 2 _ tboudreau 2010-11-10 09:30:19 UTC
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).
Comment 3 Petr Jiricka 2010-11-11 09:28:19 UTC
Why was this assigned to phejl; moreover, without any comment? The owner of RP should be primarily responsible for this.
Comment 4 Jaroslav Tulach 2010-11-11 10:41:17 UTC
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.
Comment 5 Petr Hejl 2010-11-11 13:15:47 UTC
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.
Comment 6 Petr Hejl 2010-11-11 13:42:28 UTC
(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.