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

comm: Impose an artificial bound on channel size #12981

Closed
wants to merge 2 commits into from

Conversation

alexcrichton
Copy link
Member

This commit adds an artificial and "very high" bound on the size of an unbounded
asynchronous channel. An unbounded channel runs the risk of memory exhaustions,
resulting in a process abort (currently OOM is not a fail!()). This is often a
disastrous failure mode, so instead this commit attempts to mitigate some of the
worry by imposing a large limit which will theoretically get triggered before
hitting OOM.

This commit also adds an ability to sidestep these assertions by using the
unchecked_channel method to create a Sender/Receiver pair. This function is
currently marked as #[experimental] due to leaking implementation
abstractions.

@thestinger
Copy link
Contributor

I don't think adding an arbitrary limit hard-wired limit is a good way to work around this issue. The number of channels used by an application will vary, as will the available memory. Failure is better than running out of memory, but this is just going to offer a false sense of correctness. Increasing the size of the API and the complexity of the semantics isn't free.

I think using the unbounded channels simply needs to be restricted to cases where there's a known external bound that's viewed as acceptable. The standard library should offer true bounded channels with various ways of handling a full channel (blocking, blocking with timeout, asynchronous, dropping new messages, dropping old messages) and then there's no reason to add hacks here.

Either way, as an API change that's potentially controversial I think this should be discussed.

@brson
Copy link
Contributor

brson commented Mar 17, 2014

This change was already discussed in January: https://mail.mozilla.org/pipermail/rust-dev/2014-January/007924.html

The purpose of this is to catch programmer errors where you have unexpected unbounded growth but picked the wrong way to deal with it. The next part of the plan is to add synchronous channels.

This opened up after the removal of `Chan::new`. This is just an internal
implementation detail.
This commit adds an artificial and "very high" bound on the size of an unbounded
asynchronous channel. An unbounded channel runs the risk of memory exhaustions,
resulting in a process abort (currently OOM is not a fail!()). This is often a
disastrous failure mode, so instead this commit attempts to mitigate some of the
worry by imposing a large limit which will theoretically get triggered before
hitting OOM.

This commit also adds an ability to sidestep these assertions by using the
`unchecked_channel` method to create a Sender/Receiver pair. This function is
currently marked as `#[experimental]` due to leaking implementation
abstractions.
@thestinger
Copy link
Contributor

I don't take issue with the overall proposal, just this part. It's an unreliable way of catching unbounded growth because it relies on a hard-wired magic number. It's not possible to account for the amount of available memory and the program's design.

If it's a debugging feature, then I don't think it should throw a failure. A failure can be caught, so it's a runtime feature that will be relied upon - even if it's not reliably thrown.

@alexcrichton
Copy link
Member Author

Closing due to inactivity, it does not appear that there is as much support for this as when it was originally proposed. This is easy enough to retroactively add, and it's essentially guaranteed to not be a backwards compatibility hazard.

@alexcrichton alexcrichton deleted the async-bound branch April 9, 2014 00:18
lnicola pushed a commit to lnicola/rust that referenced this pull request Aug 16, 2022
Remove imports that are also in edition 2021's prelude

small cleanup
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.

3 participants