-
Notifications
You must be signed in to change notification settings - Fork 512
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
Implement IntoFuture
for WinRT async types
#3177
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 |
---|---|---|
@@ -0,0 +1,74 @@ | ||
#![cfg(feature = "std")] | ||
|
||
use std::{ | ||
future::Future, | ||
pin::Pin, | ||
sync::{Arc, Mutex}, | ||
task::{Poll, Waker}, | ||
}; | ||
|
||
/// Wraps an `IAsyncOperation`, `IAsyncOperationWithProgress`, `IAsyncAction`, or `IAsyncActionWithProgress`. | ||
/// Impls for this trait are generated automatically by windows-bindgen. | ||
pub trait AsyncOperation { | ||
/// The type produced when the operation finishes. | ||
type Output; | ||
/// Returns whether the operation is finished, in which case `self.get_results()` can be used to get the returned data. | ||
/// Wraps `self.Status() != AsyncStatus::Started`. | ||
fn is_complete(&self) -> crate::Result<bool>; | ||
/// Register a callback that will be called once the operation is finished. | ||
/// This can only be called once. | ||
/// Wraps `self.SetCompleted(f)`. | ||
fn set_completed(&self, f: impl Fn() + Send + 'static) -> crate::Result<()>; | ||
/// Get the result value from a completed operation. | ||
/// Wraps `self.GetResults()`. | ||
fn get_results(&self) -> crate::Result<Self::Output>; | ||
/// Attempts to cancel the operation. Any error is ignored. | ||
/// Wraps `self.Cancel()`. | ||
fn cancel(&self); | ||
} | ||
|
||
/// A wrapper around an `AsyncOperation` that implements `std::future::Future`. | ||
/// This is used by generated `IntoFuture` impls. It shouldn't be necessary to use this type manually. | ||
pub struct FutureWrapper<T: AsyncOperation> { | ||
inner: T, | ||
waker: Option<Arc<Mutex<Waker>>>, | ||
} | ||
|
||
impl<T: AsyncOperation> FutureWrapper<T> { | ||
/// Creates a `FutureWrapper`, which implements `std::future::Future`. | ||
pub fn new(inner: T) -> Self { | ||
Self { inner, waker: None } | ||
} | ||
} | ||
|
||
impl<T: AsyncOperation> Unpin for FutureWrapper<T> {} | ||
|
||
impl<T: AsyncOperation> Future for FutureWrapper<T> { | ||
type Output = crate::Result<T::Output>; | ||
|
||
fn poll(mut self: Pin<&mut Self>, cx: &mut std::task::Context<'_>) -> Poll<Self::Output> { | ||
if self.inner.is_complete()? { | ||
Poll::Ready(self.inner.get_results()) | ||
} else { | ||
if let Some(saved_waker) = &self.waker { | ||
// Update the saved waker, in case the future has been transferred to a different executor. | ||
// (e.g. if using `select`.) | ||
let mut saved_waker = saved_waker.lock().unwrap(); | ||
kennykerr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
saved_waker.clone_from(cx.waker()); | ||
} else { | ||
let saved_waker = Arc::new(Mutex::new(cx.waker().clone())); | ||
self.waker = Some(saved_waker.clone()); | ||
self.inner.set_completed(move || { | ||
saved_waker.lock().unwrap().wake_by_ref(); | ||
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. For future reference, nothing appears to prevent this from racing ahead and running against a previous waker while the saved waker is waiting to be updated above. |
||
})?; | ||
} | ||
Poll::Pending | ||
} | ||
} | ||
} | ||
|
||
impl<T: AsyncOperation> Drop for FutureWrapper<T> { | ||
fn drop(&mut self) { | ||
self.inner.cancel(); | ||
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. WinRT's cancellation support isn't very widely supported and requires a fair amount of interlocking over and above the virtual function dispatch cost. I don't think we should call that here just because it is "conventional in Rust". |
||
} | ||
} |
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.
This seems like an overly expensive combination for synchronization. I'm not sure how to simplify but I'll play around with it a bit.
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.
Haven't found a simpler approach. 🤷
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.
How does this compare to the old implementation? Is this incurring an extra allocation for each Future call? :o
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.
Yes, previously (#3142) I didn't store the
Waker
at all. Now theWaker
is stored in anArc
, which adds a heap allocation, and requires a bunch of interlocking to retrieve theWaker
.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.
This is why I'm not excited about this in general - the WinRT async model and the Rust futures library don't seem to have a natural fit.
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 don't feel like an extra heap allocation is a huge problem since eg. calling SetCompleted also needs to allocate already.
I think it is possible to combine those into one allocation though at the cost of some unsafe code. Would you prefer that?
Also, would you prefer to replace the Mutex with lighter weight synchronization using hand-rolled atomics?
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'd prefer to keep it as simple as possible. We can always optimize later as needed.
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.
Perhaps at least a comment here somewhere that the
Completed
can only be set once and thus the need for a sharedWaker
.