-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
stream: add StreamExt::timeout() #2149
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alce a couple of notes:
- it looks like the feature flagging of the
timeout
method to depend on the "time" feature isn't complete? - looks like the
basic_usage
test has failed on MacOS but passed on Linux...I wonder if, rather than there being an OS-related failure, the test is flaky/non-deterministic?
Thank you @hawkw. I fixed the flaky test and feature-gated as suggested. |
There are 2 things I didn't think of when implementing this that may make sense:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 👍 I had a behavior question I wrote up inline.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍 thanks for sticking w/ this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, modulo one docs nit!
Adds a per-item timeout to
Stream