Implement IntoFuture for WinRT async types
#3177
Conversation
|
@microsoft-github-policy-service agree |
|
Yes please, we definitely use this, so we're stuck on an old windows-rs version for now 💀 |
|
Thanks, been away for a few days. Will review when I dig out from under this backlog. |
|
Adding an explanation to the readme about how and why using |
|
The clippy build failure has already been addressed - you likely just need to merge master. |
|
|
||
| impl<T: AsyncOperation> Drop for FutureWrapper<T> { | ||
| fn drop(&mut self) { | ||
| self.inner.cancel(); |
There was a problem hiding this comment.
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".
| /// 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>>>, |
There was a problem hiding this comment.
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.
Haven't found a simpler approach. 🤷
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
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.
I'd prefer to keep it as simple as possible. We can always optimize later as needed.
There was a problem hiding this comment.
Perhaps at least a comment here somewhere that the Completed can only be set once and thus the need for a shared Waker.
kennykerr
changed the title
IntoFuture for WinRT async types
|
Are you planning on clearing up the build issues? |
|
Yeah, I'll have time to poke at this again in a few days.
|
|
I'd rather not keep PRs open indefinitely. Feel free to revisit when you've had time to work through the feedback and are ready for review. |
| 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.
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.
|
I added a sketch of what I think is needed here: #342 |
This reverts #3142 while also addressing #342.
I implement
IntoFutureby wrapping eachIAsyncOperation/etc into awindows_core::FutureWrapperthat includes a mutable Waker slot (i.e.Arc<Mutex<Waker>>, so that we only need to callSetCompletedonce. To deduplicate the implementation for the various interfaces that exist, I've added a traitwindows_core::AsyncOperationthat abstracts over the SetCompleted/etc methods.I believe the
Arcis necessary as there is no way to cancel aSetCompletedcallback once it is registered. TheMutex<Waker>could be anAtomicWakerinstead, but that's a fairly complex piece of code that I don't want to pull in.Additionally, the wrapper future automatically calls
Cancel()when dropped, as is conventional for Rust futures.