Skip to content
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

Add new types of behavior for when the ScheduledThreadPool is dropped #17

Merged
merged 3 commits into from
Mar 7, 2023

Conversation

hamchapman
Copy link
Contributor

@hamchapman hamchapman commented Feb 3, 2023

Add ScheduledThreadPool::with_name_and_drop_behavior method that creates a
ScheduledThreadPool with the provided name, number of threads, and the desired
behavior in relation to pending scheduled executions when the pool is dropped.

This on drop behavior is specified using the new OnPoolDropBehavior enum.

The existing behavior (and the default) is specified as
CompletePendingScheduled. This means that all pending scheduled executions
will be run, but periodic actions will not be rescheduled once these have
completed.

The other option is DiscardPendingScheduled. As the name suggests, this
behavior means that any pending scheduled executions will be discarded (not
run).


Introduce a ScheduledThreadPoolBuilder type that allows using a builder
pattern to configure a new pool.

This builder pattern must be used if you wish to control the behavior of the
pool when it's dropped (via the builder's on_drop_behavior method).


Fix a docs typo


For what it's worth, I don't love the new with_name_and_drop_behavior method. I was tempted to add a builder pattern, of sorts, but didn't want to bloat the scope of this PR before getting feedback on this smaller change. As things stand the public API hasn't changed other than with the addition of with_name_and_drop_behavior. Updated to add a ScheduledThreadPoolBuilder

For more context, the motivation for taking a look into this is sfackler/r2d2#99. The reap_connections scheduled executions there mean that the threads are kept alive for 30s after the pool has been dropped (in the worst case). This is unfortunate especially seeing as the SharedPool in r2d2 would have already been long dropped by the time the scheduled execution ended up being run and so the None path of this code would get hit: https://github.com/sfackler/r2d2/blob/1178d1805dda0ec08f9cec626a67575691c0ce8f/src/lib.rs#L288-L291.

With the additions in this PR it would be possible to update the creation of the ScheduledThreadPool to use one of these new OnDropBehaviors, or at least allow users to specify their own ScheduledThreadPool that uses one of these new OnDropBehaviours via the thread_pool method on the builder.

src/lib.rs Outdated Show resolved Hide resolved
@sfackler
Copy link
Owner

sfackler commented Feb 3, 2023

This seems pretty reasonable to me, though I think it'd be better to use a builder rather than adding even more constructor variants.

…eates a

`ScheduledThreadPool` with the provided name, number of threads, and the desired
behavior in relation to pending scheduled executions when the pool is dropped.

This on drop behavior is specified using the new `OnPoolDropBehavior` enum.

The existing behavior (and the default) is specified as
`CompletePendingScheduled`. This means that all pending scheduled executions
will be run, but periodic actions will not be rescheduled once these have
completed.

The other option is `DiscardPendingScheduled`. As the name suggests, this
behavior means that any pending scheduled executions will be discarded (not
run).
@hamchapman
Copy link
Contributor Author

I've updated the PR to remove the RunPendingScheduledImmediately case from the OnPoolDropBehavior enum and to introduce a ScheduledThreadPoolBuilder type.

I don't know if it makes sense to have a default value for the number of threads when using the builder, which is what I've done for now (with a default value of 2), or if it makes more sense to do something like rename ScheduledThreadPool::builder() to ScheduledThreadPool::builder_with_num_threads(...) and use that to enforce that a number of threads is explicitly specified by the user. Or we could leave the num_threads field as an Option and then assert that it's not None at the time build() is called on the builder. What do you think?

@sfackler
Copy link
Owner

sfackler commented Feb 3, 2023

There are a few options:

  1. Have a default thread count. I don't think this is a great option personally.
  2. Panic in build if the thread count isn't set.
  3. Make a "staged" API for the builder so that you have to set the thread count. fn build() -> BuilderNumThreadsStage, impl BuilderNumThreadsStage { pub fn num_threads(self, num_threads: usize) -> BuilderFinalStage { ... } }, impl BuilderFinalStage { pub fn build(self) -> ScheduledThreadPool }.
  4. Have the thread count be provided in the build call.

…lder

pattern to configure a new pool.

This builder pattern must be used if you wish to control the behavior of the
pool when it's dropped (via the builder's `on_drop_behavior` method).
@hamchapman
Copy link
Contributor Author

I've updated things again. I've essentially gone with what you described in option 4 but have named the build method build_with_num_threads. I don't feel particularly strongly about which option is best (or the implementation specifics of it), but I'd lean towards either option 4 or option 2, personally.

Let me know what you think.

@hamchapman
Copy link
Contributor Author

@sfackler is there anything I can do to help get this merged?

@sfackler
Copy link
Owner

sfackler commented Mar 7, 2023

Nope, sorry for the delay!

@sfackler sfackler merged commit a8abb5b into sfackler:master Mar 7, 2023
@sfackler
Copy link
Owner

sfackler commented Mar 7, 2023

I ended up switching to a staged builder after thinking about it a bit more, but it's published now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants