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 into_inner and similar methods to StreamFuture #722

Merged
merged 1 commit into from
Feb 1, 2018

Conversation

tdyas
Copy link
Contributor

@tdyas tdyas commented Jan 30, 2018

When selecting on a StreamFuture and other futures, it is useful to be able to recover the underlying Stream if one of the other futures resolves first.

See #633 (comment) for more information.

@alexcrichton
Copy link
Member

Thanks! Looks like CI may be failing?

@tdyas tdyas force-pushed the into_inner_for_stream_future branch from 7c2a8a5 to a072a1d Compare January 30, 2018 18:13
@tdyas
Copy link
Contributor Author

tdyas commented Jan 30, 2018

Pushed a fix. Helps if I had run cargo test in the correct shell window ...

@alexcrichton
Copy link
Member

Thanks! Could these functions actually return Option though with documentation indicating when None is returned?

@tdyas
Copy link
Contributor Author

tdyas commented Jan 30, 2018

They could. That's an API design choice that is yours to make. From what I understand, the self.stream Option should be Some(S) unless the StreamFuture is being actively polled. So I see two choices:

  1. Document that the methods can return None if the future is being actively polled. The developer has the responsibility to ensure that polling has stopped (such as when the StreamFuture is returned to the closure of some other combinator). This choice won't panic of its own accord, but will if the developer tries to unilaterally unwrap the Option.

  2. Keep the implementation I wrote which will panic if the StreamFuture is in the midst of being polled.

Pushing the chance of panic to the developer seems like the better solution, although the fact that the stream is stored in an Option internally seems like an internal implementation detail of StreamFuture. I would lean to number 1 but up to you.

@alexcrichton
Copy link
Member

Yeah the convention so far that we're trying to stick to is to push the panics outwards, e.g. give you a chance to handle the case where the inner stream is gone.

When selecting on a StreamFuture and other futures, it is useful to be able
to recover the underlying Stream if one of the other futures resolves first.

These methods return `Option` to account for the fact that `StreamFuture`'s
implementation of `Future::poll` consumes the underlying stream during polling
in order to return it to the caller of `Future::poll` if the stream yieleded
an element.
@tdyas tdyas force-pushed the into_inner_for_stream_future branch from a072a1d to 767df28 Compare January 31, 2018 04:20
@tdyas
Copy link
Contributor Author

tdyas commented Feb 1, 2018

The latest change has the Option return types and applicable documentation.

@alexcrichton alexcrichton merged commit b5760e1 into rust-lang:master Feb 1, 2018
@alexcrichton
Copy link
Member

Thanks!

@tdyas tdyas deleted the into_inner_for_stream_future branch February 1, 2018 21:13
///
/// This method returns an `Option` to account for the fact that `StreamFuture`'s
/// implementation of `Future::poll` consumes the underlying stream during polling
/// in order to return it to the caller of `Future::poll` if the stream yieleded

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yielded 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR for fix: #723

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And thanks. :)

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