Skip to content

Check for STA and NA when deciding to call directly or post a callback. - #459

Merged
Kenny Kerr (kennykerr) merged 6 commits into
microsoft:masterfrom
jlaanstra:user/jlaans/458
Jan 3, 2020
Merged

Check for STA and NA when deciding to call directly or post a callback.#459
Kenny Kerr (kennykerr) merged 6 commits into
microsoft:masterfrom
jlaanstra:user/jlaans/458

Conversation

@jlaanstra

@jlaanstra Johan Laanstra (jlaanstra) commented Dec 25, 2019

Copy link
Copy Markdown
Contributor

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 Raymond Chen (@oldnewthing) Kenny Kerr (@kennykerr)

Comment thread strings/base_coroutine_threadpool.h Outdated
@kennykerr

Kenny Kerr (kennykerr) commented Dec 29, 2019

Copy link
Copy Markdown
Collaborator

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();
}

@kennykerr Kenny Kerr (kennykerr) left a comment

Copy link
Copy Markdown
Collaborator

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 Johan Laanstra (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
Copy Markdown
Contributor Author

Pushed some changes.

@kennykerr

Copy link
Copy Markdown
Collaborator

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

@jlaanstra

Copy link
Copy Markdown
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
Copy Markdown
Collaborator

And you have a clean pass from run_tests.cmd?

@jlaanstra

Johan Laanstra (jlaanstra) commented Jan 3, 2020

Copy link
Copy Markdown
Contributor Author

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>

@kennykerr Kenny Kerr (kennykerr) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

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