Skip to content

Add queue_len() option to builder, and adopt crossbeam_channel#104

Open
tarka wants to merge 2 commits into
rust-threadpool:masterfrom
tarka:add-crossbeam-and-queue_len
Open

Add queue_len() option to builder, and adopt crossbeam_channel#104
tarka wants to merge 2 commits into
rust-threadpool:masterfrom
tarka:add-crossbeam-and-queue_len

Conversation

@tarka
Copy link
Copy Markdown

@tarka tarka commented Dec 13, 2019

This can be considered an RFC patch to gauge interest/acceptance.

This adds a queue_len() option to the builder. This will limit the number of concurrent pending jobs that can be queued; attempts to queue a job when the queue is full will block until a slot becomes free. The main purpose of this feature is to allow back-pressure in the case where pending jobs use up limited resources (memory, file-handles, etc.).

In order to do this this in the simplest manner I've moved the pool to use crossbeam_channel, which has a drop-in bounded channel implementation.

There is a probably a strong case for also providing a try_execute() function that will return an error or false if the queue is full, but I've skipped that until this feature has been discussed.

@dns2utf8
Copy link
Copy Markdown
Member

Thank you for the PR.

I will look at it and see how it fits into the the new architecture.

@dns2utf8 dns2utf8 self-requested a review December 15, 2019 14:10
@dns2utf8
Copy link
Copy Markdown
Member

The reason the test fail is because the oldest rustc we support is 1.13 currently.
What is the oldest possible version crossbeam supports?

@tarka
Copy link
Copy Markdown
Author

tarka commented Dec 15, 2019

Thanks for the heads-up, I missed that.

Crossbeam Channel requires 1.28.0. I've tried testing with that, but due to a transitive dependency on cfg-if it requires enabling some 2018 features (editions and rename-dependency). In that case it may be better to just move threadpool compability to 2018/1.31 (which is over a year old now).

If the team are OK with this I'm happy to do the conversion work.

@dns2utf8
Copy link
Copy Markdown
Member

dns2utf8 commented Dec 17, 2019

I guess we will switch the crate to edition = 2018 with 2.0, but keep the 1.x release with the support of rust 1.13.x since this would be a breaking change for some people.

For 2.0 I would rise the minimal level to 1.34.2 because it is shipped with debian

Would you be interested in taking a look at the 2.0 design?

@dns2utf8 dns2utf8 added the 2.0 Currently planed for 2.0 label Dec 18, 2019
@dns2utf8 dns2utf8 mentioned this pull request Dec 18, 2019
@tarka
Copy link
Copy Markdown
Author

tarka commented Dec 20, 2019

Sure, I'll take a look this weekend.

@tarka
Copy link
Copy Markdown
Author

tarka commented Dec 20, 2019

I've had a look at it, and it all looks reasonable. Some parts may have a small user-base, so it might be worth making them enabled via feature-flag to avoid blowing out the dependency size for the default usage.

Would it be worth converting it into a PR (Rust RFC-style) and pushing to Reddit and the forum for more input? Or would it be better to just start work on a 2.0 branch and get feedback there?

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

Labels

2.0 Currently planed for 2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants