sync: oneshot::Receiver::is_terminated()#7152
Merged
maminrayej merged 3 commits intotokio-rs:masterfrom Feb 16, 2025
Merged
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
maminrayej
reviewed
Feb 15, 2025
cratelyn
added a commit
to cratelyn/tokio
that referenced
this pull request
Feb 15, 2025
> You could also add a test for `is_terminated` before and after > `try_recv` as well. tokio-rs#7152 (comment) Co-authored-by: M.Amin Rayej <m.amin.rayej@gmail.com> Signed-off-by: katelyn martin <me+cratelyn@katelyn.world>
this commit introduces a new method to `tokio::sync::oneshot::Receiver<T>`. broadly speaking, users of the oneshot channel are encouraged to `.await` the `Receiver<T>` directly, as it will only yield a single value. users implementing their own `std::future::Future`s directly may instead poll the receiver via `<Receiver<T> as Future<Output = Result<T, RecvError>::poll(..)`. note that the contract of `Future::poll()` states that clients should not poll a future after it has yielded `Poll::Ready(value)`. this commit provides a way to inspect the state of a receiver, to avoid violating the contact of `Future::poll(..)`, or requiring that a oneshot channel users track this state themselves externally via mechanisms like `futures::future::FusedFuture`, or wrapping the receiver in an `Option<T>`. NB: this makes a small behavioral change to the implementation of `<Receiver<T> as Future<Output = Result<T, RecvError>::poll(..)`. this change is acceptable, per the `Future::poll()` documentation regarding panics: > Once a future has completed (returned Ready from poll), calling its poll method again may panic, block forever, or cause other kinds of problems; the Future trait places no requirements on the effects of such a call. the upside of this is that it means a broken or closed channel, e.g. when the sender is dropped, will settle as "finished" after it yields an error. see: * tokio-rs#7137 (comment) * https://doc.rust-lang.org/stable/std/future/trait.Future.html#panics Signed-off-by: katelyn martin <me+cratelyn@katelyn.world>
> You could also add a test for `is_terminated` before and after > `try_recv` as well. tokio-rs#7152 (comment) Co-authored-by: M.Amin Rayej <m.amin.rayej@gmail.com> Signed-off-by: katelyn martin <me+cratelyn@katelyn.world>
036c002 to
8fc0759
Compare
cratelyn
added a commit
to cratelyn/tokio
that referenced
this pull request
Feb 15, 2025
NB: this commit means that this branch (tokio-rs#7153) is dependent upon interfaces proposed in (tokio-rs#7152). pr tokio-rs#7152 proposes a `is_terminated()` method for the oneshot channel's receiver. that is the proper method to use to check whether or not it is safe to poll a oneshot receiver as a future. this commit adds a note cross-referencing this method to the documentation of `is_empty()`, to help mitigate misuse. this method only reports whether a value is currently waiting in the channel; a channel that has already received and yielded a value to its receiver is also empty, but would panic when polled. Signed-off-by: katelyn martin <me+cratelyn@katelyn.world>
cratelyn
added a commit
to cratelyn/tokio
that referenced
this pull request
Feb 15, 2025
NB: this commit means that this branch (tokio-rs#7153) is dependent upon interfaces proposed in (tokio-rs#7152). pr tokio-rs#7152 proposes a `is_terminated()` method for the oneshot channel's receiver. that is the proper method to use to check whether or not it is safe to poll a oneshot receiver as a future. this commit adds a note cross-referencing this method to the documentation of `is_empty()`, to help mitigate misuse. this method only reports whether a value is currently waiting in the channel; a channel that has already received and yielded a value to its receiver is also empty, but would panic when polled. Signed-off-by: katelyn martin <me+cratelyn@katelyn.world>
maminrayej
reviewed
Feb 16, 2025
Signed-off-by: katelyn martin <me+cratelyn@katelyn.world>
maminrayej
approved these changes
Feb 16, 2025
Member
|
Thanks. |
cratelyn
added a commit
to cratelyn/tokio
that referenced
this pull request
Feb 16, 2025
NB: this commit means that this branch (tokio-rs#7153) is dependent upon interfaces proposed in (tokio-rs#7152). pr tokio-rs#7152 proposes a `is_terminated()` method for the oneshot channel's receiver. that is the proper method to use to check whether or not it is safe to poll a oneshot receiver as a future. this commit adds a note cross-referencing this method to the documentation of `is_empty()`, to help mitigate misuse. this method only reports whether a value is currently waiting in the channel; a channel that has already received and yielded a value to its receiver is also empty, but would panic when polled. Signed-off-by: katelyn martin <me+cratelyn@katelyn.world>
|
Lol this started causing some panics in production for us. Misuse on our part but it was a tricky one to root cause |
Member
|
@rushilmehra Can you describe it in a separate issue? I think this change is not supposed to affect the existing code. |
We were accidentally polling a oneshot receiver after it was complete, honestly just misuse on our side. |
|
Fortunately for us it only appeared as a flaky test 😄 Thanks for the nice PR description btw, helped finding the issue quickly :) |
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
this commit introduces a new method to
tokio::sync::oneshot::Receiver<T>.broadly speaking, users of the oneshot channel are encouraged to
.awaittheReceiver<T>directly, as it will only yield a single value.users implementing their own
std::future::Futures directly may instead poll the receiver via<Receiver<T> as Future<Output = Result<T, RecvError>::poll(..). note that the contract ofFuture::poll()states that clients should not poll a future after it has yieldedPoll::Ready(value).this commit provides a way to inspect the state of a receiver, to avoid violating the contact of
Future::poll(..), or requiring that a oneshot channel users track this state themselves externally via mechanisms likefutures::future::FusedFuture, or wrapping the receiver in anOption<T>.NB: this makes a small behavioral change to the implementation of
<Receiver<T> as Future<Output = Result<T, RecvError>::poll(..). this change is acceptable, per theFuture::poll()documentation regarding panics:the upside of this is that it means a broken or closed channel, e.g. when the sender is dropped, will settle as "finished" after it yields an error.
see:
oneshot::Receiver::{is_empty, is_closed}#7137 (comment)