New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

posting to a shutdown thread pool? #192

Closed
jrochkind opened this Issue Nov 23, 2014 · 7 comments

Comments

2 participants
@jrochkind
Contributor

jrochkind commented Nov 23, 2014

If you post to a threadpool that is in shutdown? state, your work does not get executed and the call returns false.

What I would have expected instead, is following whatever is the pool's overflow_policy -- raise an exception for :abort, run in caller thread for :caller_runs, and only return without complaining (maybe returning false, sure) for :discard.

But is the present behavior what the java.util.concurrent stuff does? Maybe they have some reason for it, I guess, and I understand wanting to stay consistent. But following the overflow_policy seems more useful and expected to me.

It is surprising to me to have to check the return value of #post to know if the work is actually going to be done, I'd expect an RejectedExcecution exception by default if posting to a shutdown pool (and since overflow_policy is abort by default, that's what I'd get if it followed overflow policy)

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Nov 24, 2014

Contributor

Looks to me like Java thread pools do what I'm suggesting -- they use the overflow policy for work submitted to a shut down pool too.

See "Rejected tasks" at:

https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ThreadPoolExecutor.html

New tasks submitted in method execute(java.lang.Runnable) will be rejected when the Executor has been shut down, and also when the Executor uses finite bounds for both maximum threads and work queue capacity, and is saturated. In either case, the execute method invokes the RejectedExecutionHandler.rejectedExecution(java.lang.Runnable, java.util.concurrent.ThreadPoolExecutor) method of its RejectedExecutionHandler. Four predefined handler policies are provided…

Contributor

jrochkind commented Nov 24, 2014

Looks to me like Java thread pools do what I'm suggesting -- they use the overflow policy for work submitted to a shut down pool too.

See "Rejected tasks" at:

https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/ThreadPoolExecutor.html

New tasks submitted in method execute(java.lang.Runnable) will be rejected when the Executor has been shut down, and also when the Executor uses finite bounds for both maximum threads and work queue capacity, and is saturated. In either case, the execute method invokes the RejectedExecutionHandler.rejectedExecution(java.lang.Runnable, java.util.concurrent.ThreadPoolExecutor) method of its RejectedExecutionHandler. Four predefined handler policies are provided…

@jdantonio jdantonio self-assigned this Nov 24, 2014

@jdantonio jdantonio added this to the 0.8.0 Release milestone Nov 24, 2014

@jdantonio

This comment has been minimized.

Show comment
Hide comment
@jdantonio

jdantonio Nov 24, 2014

Member

Nice catch. Thank you. The overflow policies are a recent addition. The current post-shutdown behavior was written first. The behavior you suggest makes more sense now that we have the overflow policies, consistency with Java's thread pools has always been a goal (Sun/Oracle has some very smart people working on this stuff). We'll make this change in the next release.

Member

jdantonio commented Nov 24, 2014

Nice catch. Thank you. The overflow policies are a recent addition. The current post-shutdown behavior was written first. The behavior you suggest makes more sense now that we have the overflow policies, consistency with Java's thread pools has always been a goal (Sun/Oracle has some very smart people working on this stuff). We'll make this change in the next release.

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Nov 24, 2014

Contributor

Oh awesome, looks like I discovered this gem right at the point when you had added enough for me to use it, I didn't realize the overflow policies were a recent addition! Awesome.

Contributor

jrochkind commented Nov 24, 2014

Oh awesome, looks like I discovered this gem right at the point when you had added enough for me to use it, I didn't realize the overflow policies were a recent addition! Awesome.

rkday added a commit to rkday/concurrent-ruby that referenced this issue Dec 7, 2014

Improve behaviour when posting to a shutdown thread pool
If an executor has a handle_overflow method defined, use that in the case where we post to an executor which is not running. If no handle_overflow method is defined (e.g. for the single-thread executor), preserve the current behaviour of returning false.

Fixes Github issue #192.
@jdantonio

This comment has been minimized.

Show comment
Hide comment
@jdantonio

jdantonio Dec 7, 2014

Member

This behavior has been implemented for pure-Ruby thread pools, but still needs implemented for the JRuby-optimized thread pools.

Member

jdantonio commented Dec 7, 2014

This behavior has been implemented for pure-Ruby thread pools, but still needs implemented for the JRuby-optimized thread pools.

@jrochkind

This comment has been minimized.

Show comment
Hide comment
@jrochkind

jrochkind Dec 8, 2014

Contributor

Awesome. I'm surprised the JRuby optimized thread pools don't already do it if they are using java.util.concurrent.ThreadExecutor under the hood which does it itself.

But if it would be helpful for me to try and help, I can try to find time to take a look and make a PR? Did you already add tests for this behavior that run against the pure-ruby version, and can be run against the Java version too to confirm? Or would be that be the place to start? Do you guys already have a test infrastructure that tests everything against multiple environments to ensure consistent behavior?

Contributor

jrochkind commented Dec 8, 2014

Awesome. I'm surprised the JRuby optimized thread pools don't already do it if they are using java.util.concurrent.ThreadExecutor under the hood which does it itself.

But if it would be helpful for me to try and help, I can try to find time to take a look and make a PR? Did you already add tests for this behavior that run against the pure-ruby version, and can be run against the Java version too to confirm? Or would be that be the place to start? Do you guys already have a test infrastructure that tests everything against multiple environments to ensure consistent behavior?

@jdantonio

This comment has been minimized.

Show comment
Hide comment
@jdantonio

jdantonio Dec 8, 2014

Member

@jrochkind The JRuby thread pools are just a thin wrapper over java.util.concurrent and those thread pools reject tasks post when the pool isn't running. @rkday has submit PR #201 which makes the corresponding update to the Java thread pools. I expect to merge that PR today.

Member

jdantonio commented Dec 8, 2014

@jrochkind The JRuby thread pools are just a thin wrapper over java.util.concurrent and those thread pools reject tasks post when the pool isn't running. @rkday has submit PR #201 which makes the corresponding update to the Java thread pools. I expect to merge that PR today.

@jdantonio

This comment has been minimized.

Show comment
Hide comment
@jdantonio

jdantonio Dec 8, 2014

Member

Completed in PRs #199, #201, and #202.

Member

jdantonio commented Dec 8, 2014

Completed in PRs #199, #201, and #202.

@jdantonio jdantonio closed this Dec 8, 2014

@jdantonio jdantonio removed the in progress label Dec 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment