Skip to content

Conversation

@jlaanstra
Copy link
Contributor

@jlaanstra jlaanstra commented Dec 25, 2019

Checking for STA when deciding to post a callback or call directly, doesn't handle the case where the apartment type is APTTYPE_NA. This PR also adds a check for NA.

Fixes #458

/cc @oldnewthing @kennykerr

@kennykerr
Copy link
Collaborator

kennykerr commented Dec 29, 2019

The logic here is a little confusing and the PR misses the subtlety, but I think we can make it work. The is_sta() check is not really about checking for the STA as much as it is checking whether a call context needs to be captured at all. Reversing then does not quite achieve the original intent, as C++/WinRT can operate in a limited way without COM being loaded (at least initially).

So I suspect you can get the desired behavior without perturbing unintended behavior simply by updating is_sta to include NA or by making is_mta mean anything that can be served by the thread pool e.g. MTA or no apartment.

For example, the PR breaks the following sample:

IAsyncAction b()
{
    co_await resume_background();
}

IAsyncAction a()
{
    co_await b();
}

int main()
{
    a().get();
}

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.

Left a few comments.

@jlaanstra jlaanstra changed the title Check for MTA instead of STA when deciding to call directly or post a callback. Check for STA and NA when deciding to call directly or post a callback. Dec 30, 2019
@jlaanstra
Copy link
Contributor Author

Pushed some changes.

@jlaanstra jlaanstra requested a review from kennykerr December 30, 2019 10:12
@kennykerr
Copy link
Collaborator

Looks good. Just checking - we've confirmed this actually works with the API in question?

@jlaanstra
Copy link
Contributor Author

Looks good. Just checking - we've confirmed this actually works with the API in question?

Correct, this change make the API in question work as expected.

@kennykerr
Copy link
Collaborator

And you have a clean pass from run_tests.cmd?

@jlaanstra
Copy link
Contributor Author

jlaanstra commented Jan 3, 2020

And you have a clean pass from run_tests.cmd?

D:\Dev\cppwinrt>run_tests.cmd x64 Release
===============================================================================
All tests passed (1972 assertions in 62 test cases)

===============================================================================
All tests passed (8 assertions in 2 test cases)

===============================================================================
All tests passed (11 assertions in 2 test cases)

===============================================================================
All tests passed (3775 assertions in 363 test cases)

===============================================================================
All tests passed (3 assertions in 1 test case)

===============================================================================
All tests passed (4 assertions in 1 test case)


D:\Dev\cppwinrt>

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.

Thanks!

@kennykerr kennykerr merged commit da87b91 into microsoft:master Jan 3, 2020
@jlaanstra jlaanstra deleted the user/jlaans/458 branch January 3, 2020 20:33
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.

Coroutine resumption logic can cause RPC_E_WRONG_THREAD exception.

3 participants