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

Abortable streams #2410

Merged
merged 9 commits into from May 10, 2021
Merged

Abortable streams #2410

merged 9 commits into from May 10, 2021

Conversation

ibraheemdev
Copy link
Contributor

@ibraheemdev ibraheemdev commented May 6, 2021

This PR makes Abortable able to work with both futures and streams.

Is there a better place to put Abortable than futures-util/src/abortable.rs? Maybe a shared/ folder?

Resolves #1424

@ibraheemdev ibraheemdev requested a review from taiki-e as a code owner May 6, 2021
Copy link
Member

@taiki-e taiki-e left a comment

Thanks! This looks good to me!

Is there a better place to put Abortable than futures-util/src/abortable.rs? Maybe a shared/ folder?

shared/ is confusing with future/shared, so I think futures-util/src/abortable.rs is fine.

futures-util/src/abortable.rs Outdated Show resolved Hide resolved
/// # use futures::stream::{self, StreamExt};
///
/// let (abort_handle, abort_registration) = AbortHandle::new_pair();
/// let mut stream = Abortable::stream(stream::iter(vec![1, 2, 3]), abort_registration);
Copy link
Member

@taiki-e taiki-e May 6, 2021

Choose a reason for hiding this comment

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

This function doesn't seem to exist, but I think it's not actually a bad idea to have both Abortable::new (require T: Future) and Abortable::stream (require T: Stream) because we can prevent the creation of Abortables that cannot be polled. -- However, it may feel redundant when we provide other abortables (e.g., abortable sink) in the future.

Copy link
Contributor Author

@ibraheemdev ibraheemdev May 6, 2021

Choose a reason for hiding this comment

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

At first I wrote Abortable::new and stream, but I thought that new for abortable futures specifically was a bit arbitrary.

@taiki-e taiki-e added 0.3-backport: pending A-stream labels May 6, 2021
futures-util/src/abortable.rs Outdated Show resolved Hide resolved
/// Checks whether the task has been aborted. See [`AbortHandle::abort`] for details.
pub fn is_aborted(&self) -> bool {
self.inner.aborted.load(Ordering::Relaxed)
}
Copy link
Member

@taiki-e taiki-e May 7, 2021

Choose a reason for hiding this comment

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

This is actually a method that returns true if it is notified that the task should be aborted (= abort was called), and I think there are some notes that are desirable to explain in the documentation.

  • This will return true even if abort is called after the task has completed.
  • This will return true even if the task still is running at the time.

Copy link
Member

@taiki-e taiki-e May 7, 2021

Choose a reason for hiding this comment

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

Alternatively, we can omit is_aborted method until someone actually requests it.

Copy link
Contributor Author

@ibraheemdev ibraheemdev May 7, 2021

Choose a reason for hiding this comment

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

We might as well keep it, because we can use it internally in try_poll instead of loading the flag manually. I'll update the docs.

Copy link
Contributor Author

@ibraheemdev ibraheemdev May 9, 2021

Choose a reason for hiding this comment

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

Updated the docs, is Ordering::Relaxed enough for the user facing API?

Copy link
Member

@taiki-e taiki-e left a comment

Thanks. LGTM aside from a nit.

futures-util/src/abortable.rs Outdated Show resolved Hide resolved
Co-authored-by: Taiki Endo <te316e89@gmail.com>
@taiki-e taiki-e merged commit d796617 into rust-lang:master May 10, 2021
20 checks passed
taiki-e pushed a commit that referenced this issue May 10, 2021
@taiki-e taiki-e mentioned this pull request May 10, 2021
@taiki-e taiki-e added 0.3-backport: completed and removed 0.3-backport: pending labels May 10, 2021
taiki-e pushed a commit that referenced this issue May 10, 2021
@taiki-e taiki-e mentioned this pull request May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants