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

Implement IntoFuture for WinRT async types #3177

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions crates/libs/bindgen/src/rust/writer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -697,6 +697,34 @@ impl Writer {
self.GetResults()
}
}
#features
impl<#constraints> windows_core::AsyncOperation for #ident {
type Output = #return_type;
fn is_complete(&self) -> windows_core::Result<bool> {
Ok(self.Status()? != #namespace AsyncStatus::Started)
}
fn set_completed(&self, f: impl Fn() + Send + 'static) -> windows_core::Result<()> {
self.SetCompleted(&#namespace #handler::new(move |_sender, _args| {
f();
Ok(())
}))
}
fn get_results(&self) -> windows_core::Result<Self::Output> {
self.GetResults()
}
fn cancel(&self) {
let _ = self.Cancel();
}
}
#features
impl<#constraints> std::future::IntoFuture for #ident {
type Output = windows_core::Result<#return_type>;
type IntoFuture = windows_core::FutureWrapper<#ident>;

fn into_future(self) -> Self::IntoFuture {
windows_core::FutureWrapper::new(self)
}
}
}
}
}
Expand Down
74 changes: 74 additions & 0 deletions crates/libs/core/src/future.rs
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>>>,
Copy link
Collaborator

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.

Copy link
Collaborator

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. 🤷

Copy link
Contributor

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

Copy link
Collaborator

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 the Waker is stored in an Arc, which adds a heap allocation, and requires a bunch of interlocking to retrieve the Waker.

Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Collaborator

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.

Copy link
Collaborator

@kennykerr kennykerr Aug 9, 2024

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 shared 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();
Copy link
Collaborator

@kennykerr kennykerr Aug 19, 2024

Choose a reason for hiding this comment

The 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();
Copy link
Collaborator

Choose a reason for hiding this comment

The 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".

}
}
2 changes: 2 additions & 0 deletions crates/libs/core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ pub mod imp;

mod as_impl;
mod com_object;
mod future;
mod guid;
mod inspectable;
mod interface;
Expand All @@ -41,6 +42,7 @@ mod weak;

pub use as_impl::*;
pub use com_object::*;
pub use future::*;
pub use guid::*;
pub use inspectable::*;
pub use interface::*;
Expand Down
162 changes: 162 additions & 0 deletions crates/libs/windows/src/Windows/Devices/Sms/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1036,6 +1036,33 @@ impl DeleteSmsMessageOperation {
}
}
#[cfg(feature = "deprecated")]
impl windows_core::AsyncOperation for DeleteSmsMessageOperation {
type Output = ();
fn is_complete(&self) -> windows_core::Result<bool> {
Ok(self.Status()? != super::super::Foundation::AsyncStatus::Started)
}
fn set_completed(&self, f: impl Fn() + Send + 'static) -> windows_core::Result<()> {
self.SetCompleted(&super::super::Foundation::AsyncActionCompletedHandler::new(move |_sender, _args| {
f();
Ok(())
}))
}
fn get_results(&self) -> windows_core::Result<Self::Output> {
self.GetResults()
}
fn cancel(&self) {
let _ = self.Cancel();
}
}
#[cfg(feature = "deprecated")]
impl std::future::IntoFuture for DeleteSmsMessageOperation {
type Output = windows_core::Result<()>;
type IntoFuture = windows_core::FutureWrapper<DeleteSmsMessageOperation>;
fn into_future(self) -> Self::IntoFuture {
windows_core::FutureWrapper::new(self)
}
}
#[cfg(feature = "deprecated")]
#[repr(transparent)]
#[derive(PartialEq, Eq, Debug, Clone)]
pub struct DeleteSmsMessagesOperation(windows_core::IUnknown);
Expand Down Expand Up @@ -1122,6 +1149,33 @@ impl DeleteSmsMessagesOperation {
}
}
#[cfg(feature = "deprecated")]
impl windows_core::AsyncOperation for DeleteSmsMessagesOperation {
type Output = ();
fn is_complete(&self) -> windows_core::Result<bool> {
Ok(self.Status()? != super::super::Foundation::AsyncStatus::Started)
}
fn set_completed(&self, f: impl Fn() + Send + 'static) -> windows_core::Result<()> {
self.SetCompleted(&super::super::Foundation::AsyncActionCompletedHandler::new(move |_sender, _args| {
f();
Ok(())
}))
}
fn get_results(&self) -> windows_core::Result<Self::Output> {
self.GetResults()
}
fn cancel(&self) {
let _ = self.Cancel();
}
}
#[cfg(feature = "deprecated")]
impl std::future::IntoFuture for DeleteSmsMessagesOperation {
type Output = windows_core::Result<()>;
type IntoFuture = windows_core::FutureWrapper<DeleteSmsMessagesOperation>;
fn into_future(self) -> Self::IntoFuture {
windows_core::FutureWrapper::new(self)
}
}
#[cfg(feature = "deprecated")]
#[repr(transparent)]
#[derive(PartialEq, Eq, Debug, Clone)]
pub struct GetSmsDeviceOperation(windows_core::IUnknown);
Expand Down Expand Up @@ -1211,6 +1265,33 @@ impl GetSmsDeviceOperation {
}
}
#[cfg(feature = "deprecated")]
impl windows_core::AsyncOperation for GetSmsDeviceOperation {
type Output = SmsDevice;
fn is_complete(&self) -> windows_core::Result<bool> {
Ok(self.Status()? != super::super::Foundation::AsyncStatus::Started)
}
fn set_completed(&self, f: impl Fn() + Send + 'static) -> windows_core::Result<()> {
self.SetCompleted(&super::super::Foundation::AsyncOperationCompletedHandler::new(move |_sender, _args| {
f();
Ok(())
}))
}
fn get_results(&self) -> windows_core::Result<Self::Output> {
self.GetResults()
}
fn cancel(&self) {
let _ = self.Cancel();
}
}
#[cfg(feature = "deprecated")]
impl std::future::IntoFuture for GetSmsDeviceOperation {
type Output = windows_core::Result<SmsDevice>;
type IntoFuture = windows_core::FutureWrapper<GetSmsDeviceOperation>;
fn into_future(self) -> Self::IntoFuture {
windows_core::FutureWrapper::new(self)
}
}
#[cfg(feature = "deprecated")]
#[repr(transparent)]
#[derive(PartialEq, Eq, Debug, Clone)]
pub struct GetSmsMessageOperation(windows_core::IUnknown);
Expand Down Expand Up @@ -1299,6 +1380,33 @@ impl GetSmsMessageOperation {
self.GetResults()
}
}
#[cfg(feature = "deprecated")]
impl windows_core::AsyncOperation for GetSmsMessageOperation {
type Output = ISmsMessage;
fn is_complete(&self) -> windows_core::Result<bool> {
Ok(self.Status()? != super::super::Foundation::AsyncStatus::Started)
}
fn set_completed(&self, f: impl Fn() + Send + 'static) -> windows_core::Result<()> {
self.SetCompleted(&super::super::Foundation::AsyncOperationCompletedHandler::new(move |_sender, _args| {
f();
Ok(())
}))
}
fn get_results(&self) -> windows_core::Result<Self::Output> {
self.GetResults()
}
fn cancel(&self) {
let _ = self.Cancel();
}
}
#[cfg(feature = "deprecated")]
impl std::future::IntoFuture for GetSmsMessageOperation {
type Output = windows_core::Result<ISmsMessage>;
type IntoFuture = windows_core::FutureWrapper<GetSmsMessageOperation>;
fn into_future(self) -> Self::IntoFuture {
windows_core::FutureWrapper::new(self)
}
}
#[cfg(all(feature = "Foundation_Collections", feature = "deprecated"))]
#[repr(transparent)]
#[derive(PartialEq, Eq, Debug, Clone)]
Expand Down Expand Up @@ -1407,6 +1515,33 @@ impl GetSmsMessagesOperation {
self.GetResults()
}
}
#[cfg(all(feature = "Foundation_Collections", feature = "deprecated"))]
impl windows_core::AsyncOperation for GetSmsMessagesOperation {
type Output = super::super::Foundation::Collections::IVectorView<ISmsMessage>;
fn is_complete(&self) -> windows_core::Result<bool> {
Ok(self.Status()? != super::super::Foundation::AsyncStatus::Started)
}
fn set_completed(&self, f: impl Fn() + Send + 'static) -> windows_core::Result<()> {
self.SetCompleted(&super::super::Foundation::AsyncOperationWithProgressCompletedHandler::new(move |_sender, _args| {
f();
Ok(())
}))
}
fn get_results(&self) -> windows_core::Result<Self::Output> {
self.GetResults()
}
fn cancel(&self) {
let _ = self.Cancel();
}
}
#[cfg(all(feature = "Foundation_Collections", feature = "deprecated"))]
impl std::future::IntoFuture for GetSmsMessagesOperation {
type Output = windows_core::Result<super::super::Foundation::Collections::IVectorView<ISmsMessage>>;
type IntoFuture = windows_core::FutureWrapper<GetSmsMessagesOperation>;
fn into_future(self) -> Self::IntoFuture {
windows_core::FutureWrapper::new(self)
}
}
#[cfg(feature = "deprecated")]
#[repr(transparent)]
#[derive(PartialEq, Eq, Debug, Clone)]
Expand Down Expand Up @@ -1493,6 +1628,33 @@ impl SendSmsMessageOperation {
self.GetResults()
}
}
#[cfg(feature = "deprecated")]
impl windows_core::AsyncOperation for SendSmsMessageOperation {
type Output = ();
fn is_complete(&self) -> windows_core::Result<bool> {
Ok(self.Status()? != super::super::Foundation::AsyncStatus::Started)
}
fn set_completed(&self, f: impl Fn() + Send + 'static) -> windows_core::Result<()> {
self.SetCompleted(&super::super::Foundation::AsyncActionCompletedHandler::new(move |_sender, _args| {
f();
Ok(())
}))
}
fn get_results(&self) -> windows_core::Result<Self::Output> {
self.GetResults()
}
fn cancel(&self) {
let _ = self.Cancel();
}
}
#[cfg(feature = "deprecated")]
impl std::future::IntoFuture for SendSmsMessageOperation {
type Output = windows_core::Result<()>;
type IntoFuture = windows_core::FutureWrapper<SendSmsMessageOperation>;
fn into_future(self) -> Self::IntoFuture {
windows_core::FutureWrapper::new(self)
}
}
#[repr(transparent)]
#[derive(PartialEq, Eq, Debug, Clone)]
pub struct SmsAppMessage(windows_core::IUnknown);
Expand Down
Loading
Loading