Windows-rs: E_ILLEGAL_DELEGATE_ASSIGNMENT when using futures::future::join_all on multiple IAsyncOperations

[chenxiaolong]

I was testing the newly added async support and noticed that when using futures::future::join_all on multiple IAsyncOperations, I'm getting the E_ILLEGAL_DELEGATE_ASSIGNMENT errors frequently (but not always).

As an example, the following works every time in C#:
```C#
static async Task Main(string[] args)
{
var devices = await Task.WhenAll(args.Select(a => BluetoothLEDevice.FromBluetoothAddressAsync(ParseBtAddress(a)).AsTask()));
Array.ForEach(devices, Console.WriteLine);
}

but the following in Rust:
```rust
async fn async_main() {
    let futures = std::env::args().skip(1)
        .map(|arg| {
            let addr = parse_bt_address(&arg);
            BluetoothLEDevice::from_bluetooth_address_async(addr).unwrap()
        });

    let results = join_all(futures).await;
    println!("{:?}", results);
}

frequently fails with something like:

[Ok(BluetoothLEDevice(Some(0x197cde26070))), Err(Error { code: 0x80000018, message: "A delegate was assigned when not allowed." })]

The full sample programs are available here:

• C#: https://gist.github.com/chenxiaolong/ce5a924357212599b579928ba9c38e2d#file-async_csharp-cs
• Rust: https://gist.github.com/chenxiaolong/ce5a924357212599b579928ba9c38e2d#file-async_rust-rs

I'm testing with ./<prog>.exe aa:bb:cc:dd:ee:ff 00:11:22:33:44:55 (except with valid bluetooth addresses). I'm using the latest commit of winrt-rs (currently 2edcccde71df2a33bd4fe5355751b8507a9d5b49). With the Rust version, sometimes the first future errors out, sometimes the second. Very rarely, both succeed.

Any ideas what might be going on?

[kennykerr]

The WinRT async pattern forbids multiple completed callbacks. The second atttempt to set a completed callback returns E_ILLEGAL_DELEGATE_ASSIGNMENT. C# avoids this by wrapping and abstracting away the async pattern entirely. This is convenient but rather costly. I'll look into why this is happening in Rust. It may be that the Future trait implementation may need a check to avoid this in some cases.

https://github.com/microsoft/winrt-rs/blob/2edcccde71df2a33bd4fe5355751b8507a9d5b49/crates/gen/src/types/futures.rs#L93-L96

[rylev]

Would using Once::call_once be a way to ensure that the callback is set only once?

[kennykerr]

Possibly, I'd just like to understand why poll is being called twice to cause this to happen. Ideally, the Future is only called once and then the Waker brings it back to life.

Also, I'm not sure where we'd store the Once since we cannot increase the size of the async object itself.

[adumbidiot]

I asked on the Rust Community Server, and apparently Future executors are allowed to poll Futures without wakeups. The join_all function in particular takes advantage of this and polls every future given to it when even one is woken up, leading to the callback being registered twice. Furthermore, every call to poll must update stored Waker values, like the one in the callback handler. The Rust Async Book confirms this. I also created a simple example on Rust Playground to demonstrate this behavior.

Since we need to somehow update the Waker inside the callback, simply using Once is not enough. Instead of dealing with Wakers direcly, maybe we could use something like futures's oneshot channel to bridge the gap between Winrt's and Rust's async systems. However, that we would need a place to store the reciever. I think it would make the most sense to provide something like a get_future method that returns a Rust struct that wraps an IAsyncOperation instead of directly implementing Future for IAsyncOperations.

[kennykerr]

Yes, it may be best simply to hide the IAsyncXxx from Rust completely and only expose a Rust-friendly wrapper at the API boundary. Sorry for the delay, I've been occupied with other issues but I hope to look at this issue again soon.

[kennykerr]

I came up with a simpler fix: https://github.com/microsoft/winrt-rs/pull/340

[adumbidiot]

That looks like a good fix for the short term. In the future though we should find a way to somehow update the Waker in that handler function to properly integrate with the Rust async system.

[kennykerr]

The waker will still be used - just from the first poll.

[adumbidiot]

According to the Rust Async book, the Waker used in future polls may be different from the one from past polls.

The import part:
"Importantly, we have to update the Waker every time the future is polled because the future may have moved to a different task with a different Waker. This will happen when futures are passed around between tasks after being polled."

So I guess if you pass the current futures between tasks, they'll just won't be polled and never complete.

[kennykerr]

Hmm that seems very implementation specific. 😕 Hopefully, this works reliably in practice.

[kennykerr]

Well it's hopefully an improvement. Do take it for a spin and let me know how it works. We can continue to improve it as needed.

[adumbidiot]

Hmm that seems very implementation specific.

Yeah it seems kind of annoying. I think the general idea was to give future executor devs a lot of freedom since there isn't really one target runtime for futures.

Well it's hopefully an improvement. Do take it for a spin and let me know how it works. We can continue to improve it as needed.

Ok, thanks for the fix!