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

resume_agile to allow coroutine to resume in any apartment #1356

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

oldnewthing
Copy link
Member

By default, co_await'ing a Windows Runtime asynchronous operation resumes in the same COM apartment. We provide a way to override this behavior and resume in any apartment by wrapping the asynchronous operation in resume_agile().

extern IAsyncAction Example();

// This resumes in the same apartment
co_await Example();

// This resumes in any apartment
co_await resume_agile(Example());

The guts of the change are in disconnect_aware_handler which accepts a preserve_context template parameter, defaulting to true. If false, then we bypass the resume_apartment() on completion. The false value is provided by the resume_agile() function.

To remove the resume_apartment_context from the disconnect_aware_handler in the case where we don't need it, I use EBO and make it a base class, so that it disappears when an empty ignore_apartment_context is used.

While I was there, I introduced movable_primitive<T> which provides RAII-like functionality to scalars like integers and pointers. This allows us to simplify disconnect_aware_handler and resume_apartment_context, which previously had to override all the copy and move operations in order to reset the scalar on move.

Also fix the await_adapter.cpp tests so they don't leak DispatcherQueues, by explicitly shutting down all the dispatcher queues we created. This required moving all the controllers to top-level so we can shut them down after the tests have completed.

Fixes: #1355

By default, `co_await`'ing a Windows Runtime asynchronous operation
resumes in the same COM apartment. We provide a way to override
this behavior and resume in any apartment by wrapping the
asynchronous operation in `resume_agile()`.

```cpp
extern IAsyncAction Example();

// This resumes in the same apartment
co_await Example();

// This resumes in any apartment
co_await resume_agile(Example());
```

The guts of the change are in `disconnect_aware_handler`
which accepts a `preserve_context` template parameter,
defaulting to `true`. If `false`, then we bypass the
`resume_apartment()` on completion. The `false` value
is provided by the `resume_agile()` function.

To remove the `resume_apartment_context` from the
`disconnect_aware_handler` in the case where we
don't need it, I use EBO and make it a base class,
so that it disappears when an empty `ignore_apartment_context`
is used.

While I was there, I introduced `movable_primitive<T>` which
provides RAII-like functionality to scalars like integers
and pointers. This allows us to simplify `disconnect_aware_handler`
and `resume_apartment_context`, which previously had to override
all the copy and move operations in order to reset the scalar
on move.

Also fix the `await_adapter.cpp` tests so they don't leak DispatcherQueues,
by explicitly shutting down all the dispatcher queues we created.
This required moving all the controllers to top-level so we can shut
them down after the tests have completed.
@oldnewthing
Copy link
Member Author

oldnewthing commented Sep 7, 2023

Open issues:

  • What should we call this thing? Is it resume_agile()? resume_any_apartment()? Something else?
  • Should it be a free function or a member? (Or both?)
Name Free Member
resume_agile co_await winrt::resume_agile(Something()); co_await Something().resume_agile();
resume_any_apartment co_await winrt::resume_any_apartment(Something()); co_await Something().resume_any_apartment();

Note that C# uses await Something().ConfigureAwait(false);, going for the "member function" column, though the name "ConfigureAwait" is quite bad.

  • Should we capture the IAsyncXxx by value or reference? Reference is more efficient, but creates the risk of UAF:
// Don't do this.
auto later = resume_agile(Something());
co_await later;

I think reference is okay because resume_foreground has the same UAF problem and nobody appears to have been badly bitten by it.

Pros/cons:

  • Member function prevents you from applying resume_agile to things that don't work.
    • co_await resume_agile(resume_background()) - oops
  • C++/WinRT doesn't inject member functions often.
  • Member functions might be more discoverable?

@jonwis
Copy link
Member

jonwis commented Sep 7, 2023

My vote is for free function. Our policy so far has to put these things as free functions so discoverability doesn't seem like a problem here ... resume_foreground, cancellation helpers, etc. If you want this, you know you need it, so you know where to find it. (I'm of two minds about monkeypatching via extension methods for the same reason.)

Can you capture it by move, so someone who says co_await resume_agile(ZorpAsync()) gets the best results?

I like resume_agile() as the naming.

Copy link
Collaborator

@kennykerr kennykerr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kennykerr kennykerr merged commit fac72c8 into microsoft:master Sep 12, 2023
75 checks passed
@oldnewthing
Copy link
Member Author

Can you capture it by move, so someone who says co_await resume_agile(ZorpAsync()) gets the best results?

I like this idea - it gives us the best of both worlds.

  • co_await resume_async(ZorpAsync()) avoids extraneous addref/release.
  • IAsyncAction op = ZorpAsync(); co_await resume_async(op); performs the necessary addref/release - avoids UAF.
  • IAsyncAction op = ZorpAsync(); co_await resume_async(std::move(op)); transfers ownership to resume_await and avoids UAF.

@oldnewthing oldnewthing deleted the resume-agile branch September 12, 2023 19:19
@sylveon
Copy link
Contributor

sylveon commented Sep 12, 2023

await_adapter is not owning, it holds Async const&

@oldnewthing
Copy link
Member Author

oldnewthing commented Sep 12, 2023

await_adapter is not owning, it holds Async const&

The difference is that up until now, await_adapter had been produced only by operator co_await, so you knew that the await_adapter was going to be awaited immediately, before the destruction of any temporaries.

This change introduces a way to obtain an await_adapter without awaiting it, namely by doing

auto oops = resume_agile(ZorpAsync());
co_await oops;

@sylveon
Copy link
Contributor

sylveon commented Sep 12, 2023

That is a fair assessment, though one could already get an adapter without awaiting by calling the operator manually.

To make this work, await_adapter and all operators would need to be changed to take ownership, not just resume_agile.

@oldnewthing
Copy link
Member Author

Somebody who invokes the operator manually auto oops = operator co_await(ZorpAsync()) knows that they are holding a live wire.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Overriding "resume in same apartment" behavior for co_await IAsyncXxx
5 participants