Fix access violation in resume_after and resume_on_signal

[sylveon]

Fixes #1329

This fixes the issue by using m_state before suspending, rather than after. The issue occurs because the callback could fire before our thread of execution resumes, causing the timespan_awaiter/signal_awaiter to be destroyed inside the coroutine frame before m_state is accessed.

As a drive-by improvement, currently if await_suspend is called with a non-idle state, the threadpool object is closed (cancelling the timer/wait), the existing coroutine handle is just dropped, and resume on the new handle is fired immediately. This would cause the existing pending coroutine to hang forever. Instead, avoid doing anything and throw an exception when the awaiter is not idle. This is a very unlikely event, the test does some gymnastics (reused awaiter) to achieve this state, but better safe than sorry.

[kennykerr]

CI build failing with:

C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\include\atomic(702) : Assertion failed: Invalid memory order

[sylveon]

Yep, taking a look

[sylveon]

A driver update hosed my system install (I love Windows), will take a bit more time than expected

[sylveon]

I stopped trying to be clever and just used a mutex, that way we can be sure that await_suspend completes before the threadpool callback resumes the coroutine.

I also took the opportunity to refactor the code to use CRTP and share implementation between timers and waits, as well as insert some useful short circuits in case of cancellation.

[sylveon]

Everything should work now :)

[kennykerr]

@oldnewthing are you able to review this?

[oldnewthing]

@kennykerr Will try to get to it, but kind of busy with other API stuff.

[github-actions[bot]]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sylveon]

Bump

[github-actions[bot]]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sylveon]

Can someone reopen?

[github-actions[bot]]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sylveon]

Bump

[github-actions[bot]]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sylveon]

@oldnewthing have you been able to take a look?

[kennykerr]

Practically, all of the thread and coroutine helpers should just move to WIL en masse. All C++/WinRT should provide are the coroutine traits for the core WinRT async types. Many of the helpers already have some equivalent or replacement. Once they're all present in WIL, I'd be happy with simply removing all of these helpers from C++/WinRT. That way C++/WinRT can focus on being a language projection driven by metadata - which is what it is - and WIL can focus on being a library - which is what it is.

[sylveon]

Alright, I'll work on porting this to wil - but we really should start marking these with deprecation notices and pointing towards the updated wil wrappers, because as it stands many users won't even be aware of the move to WIL until they are removed and would be surprised by such removal.

[sylveon]

Porting this to will would require making the cancellable awaiter stuff public, otherwise this would regress cancellation scenarios.

[github-actions[bot]]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sylveon]

I don't think this can easily be ported to WIL without making the cancellation features public (so WIL can tap into it), or porting all of the async support machinery to WIL.

What's the preference here?

[github-actions[bot]]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[github-actions[bot]]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sylveon]

Is there any plan to take a look at this PR?

[github-actions[bot]]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[github-actions[bot]]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sylveon]

Bump

[github-actions[bot]]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[github-actions[bot]]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sylveon]

Is there anything preventing this from being merged?

[kennykerr]

I'm not able to review this code https://github.com/microsoft/cppwinrt/issues/1329#issuecomment-1638440415. My preference is to strip coroutine support in C++/WinRT down to the basics, removing all of the helpers, cancellation, and progress support as it all appears somewhat unreliable and buggy and is too difficult to reason about.

[sylveon]

Migrating all of that to WIL seems preferred (I'm not opposed to that), but then it would be a breaking change that requires C++/WinRT 3. Maybe @oldnewthing can review this until then

[oldnewthing]

The PR description says "This fixes the issue by using m_state before suspending, rather than after. " But the change is much bigger than this sentence suggests.

Can we untangle the essential fix from the drive-by fix?

[sylveon]

Making the fix separately in both classes would not lead to a much better PR diff unfortunately, so I merged them together into a single base class to reduce code duplication (and therefore less effort to review).

[github-actions[bot]]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[sylveon]

In short, I don't think untangling the drive-by would make it much easier to read.

[github-actions[bot]]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[github-actions[bot]]

This pull request is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

[kennykerr]

Closing this PR as there's been no real activity in nearly 8 months. If a project maintainer is available to review, they can always reopen this PR as needed.

[JaiganeshKumaran]

Why hasn't Microsoft got anyone to look after C++/WinRT?