-
Notifications
You must be signed in to change notification settings - Fork 668
Add private fields to public types for forward compatibility #1841
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ use crate::task::AtomicWaker; | |
| use futures_core::future::Future; | ||
| use futures_core::task::{Context, Poll}; | ||
| use pin_utils::unsafe_pinned; | ||
| use core::fmt; | ||
| use core::pin::Pin; | ||
| use core::sync::atomic::{AtomicBool, Ordering}; | ||
| use alloc::sync::Arc; | ||
|
|
@@ -30,12 +31,12 @@ impl<Fut> Abortable<Fut> where Fut: Future { | |
| /// | ||
| /// ``` | ||
| /// # futures::executor::block_on(async { | ||
| /// use futures::future::{Abortable, AbortHandle, Aborted}; | ||
| /// use futures::future::{Abortable, AbortHandle}; | ||
| /// | ||
| /// let (abort_handle, abort_registration) = AbortHandle::new_pair(); | ||
| /// let future = Abortable::new(async { 2 }, abort_registration); | ||
| /// abort_handle.abort(); | ||
| /// assert_eq!(future.await, Err(Aborted)); | ||
| /// assert!(future.await.is_err()); | ||
| /// # }); | ||
| /// ``` | ||
| pub fn new(future: Fut, reg: AbortRegistration) -> Self { | ||
|
|
@@ -70,12 +71,12 @@ impl AbortHandle { | |
| /// | ||
| /// ``` | ||
| /// # futures::executor::block_on(async { | ||
| /// use futures::future::{Abortable, AbortHandle, Aborted}; | ||
| /// use futures::future::{Abortable, AbortHandle}; | ||
| /// | ||
| /// let (abort_handle, abort_registration) = AbortHandle::new_pair(); | ||
| /// let future = Abortable::new(async { 2 }, abort_registration); | ||
| /// abort_handle.abort(); | ||
| /// assert_eq!(future.await, Err(Aborted)); | ||
| /// assert!(future.await.is_err()); | ||
| /// # }); | ||
| /// ``` | ||
| pub fn new_pair() -> (Self, AbortRegistration) { | ||
|
|
@@ -121,16 +122,24 @@ pub fn abortable<Fut>(future: Fut) -> (Abortable<Fut>, AbortHandle) | |
| } | ||
|
|
||
| /// Indicator that the `Abortable` future was aborted. | ||
| #[derive(Copy, Clone, Debug, Eq, PartialEq)] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as with |
||
| pub struct Aborted; | ||
| #[derive(Clone, Eq, PartialEq)] | ||
| pub struct Aborted { | ||
| _priv: (), | ||
| } | ||
|
|
||
| impl fmt::Debug for Aborted { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| f.debug_tuple("Aborted").finish() | ||
| } | ||
| } | ||
|
|
||
| impl<Fut> Future for Abortable<Fut> where Fut: Future { | ||
| type Output = Result<Fut::Output, Aborted>; | ||
|
|
||
| fn poll(mut self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> { | ||
| // Check if the future has been aborted | ||
| if self.inner.cancel.load(Ordering::Relaxed) { | ||
| return Poll::Ready(Err(Aborted)) | ||
| return Poll::Ready(Err(Aborted { _priv: () })) | ||
| } | ||
|
|
||
| // attempt to complete the future | ||
|
|
@@ -146,7 +155,7 @@ impl<Fut> Future for Abortable<Fut> where Fut: Future { | |
| // Checking with `Relaxed` is sufficient because `register` introduces an | ||
| // `AcqRel` barrier. | ||
| if self.inner.cancel.load(Ordering::Relaxed) { | ||
| return Poll::Ready(Err(Aborted)) | ||
| return Poll::Ready(Err(Aborted { _priv: () })) | ||
| } | ||
|
|
||
| Poll::Pending | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,13 +55,19 @@ fn resolving_errors() { | |
|
|
||
| drop(tx2); | ||
|
|
||
| assert_eq!(Poll::Ready(Some(Err(oneshot::Canceled))), queue.poll_next_unpin(cx)); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the bad thing about this PR is that we cannot do this.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However, actually, this is not just a problem for this PR. I also encountered this in #1785. |
||
| match queue.poll_next_unpin(cx) { | ||
| Poll::Ready(Some(Err(_))) => {} | ||
| _ => unreachable!(), | ||
| } | ||
| assert!(!queue.poll_next_unpin(cx).is_ready()); | ||
|
|
||
| drop(tx1); | ||
| tx3.send("world2").unwrap(); | ||
|
|
||
| assert_eq!(Poll::Ready(Some(Err(oneshot::Canceled))), queue.poll_next_unpin(cx)); | ||
| match queue.poll_next_unpin(cx) { | ||
| Poll::Ready(Some(Err(_))) => {} | ||
| _ => unreachable!(), | ||
| } | ||
| assert_eq!(Poll::Ready(Some(Ok("world2"))), queue.poll_next_unpin(cx)); | ||
| assert_eq!(Poll::Ready(None), queue.poll_next_unpin(cx)); | ||
| })); | ||
|
|
||
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.
I actually like this one as-is because it allows you to easily write it as a pattern-- e.g.
I can't really imagine what else we'd want to add to it in the future.
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.
Okay, these are in util/channel, so we can change them if we need them.
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.
FWIW, I tend to write it like:
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.
@seanmonstar That doesn't actually assert that cancelled is what happened there, though-- you could be silencing a real error by accident.