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.

Sign up for GitHub

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

Jump to bottom

winrt::apartment_context considered harmful #12982

Closed
zadjii-msft opened this issue Apr 26, 2022 · 1 comment · Fixed by #15725
Closed

winrt::apartment_context considered harmful #12982

zadjii-msft opened this issue Apr 26, 2022 · 1 comment · Fixed by #15725
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Milestone
Engineering Impro...

Comments

@zadjii-msft
Copy link
Member

We should try to avoid using winrt::apartment_context as it's inherently flawed unfortunately. 😥
As Raymond Chen wrote:

Unfortunately, it's true. co_await'ing an apartment_context is blocking [the background thread]. That's because it's built out of COM's IContextCallback, which supports only synchronous transfer.
[...]
Note that it's even worse: Even after you get to the other side, the side you came *from* is still blocked until you finish doing your other-side work (which could take the form of another co_await).
[...]
If the UI thread is hung, this will lead to threadpool starvation and eventually a deadlock.

_GetWindowLayoutAsync() in particular is quite dangerous because it uses an apartment_context to switch back to a background thread and blocks the UI thread for that duration...

Originally posted by @lhecker in #12979 (comment)
@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Apr 26, 2022
@zadjii-msft
Copy link
Member Author

We should fix any uses of that.

@zadjii-msft zadjii-msft added Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. labels Apr 26, 2022
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Apr 26, 2022
@zadjii-msft zadjii-msft added this to the Engineering Improvements milestone Apr 26, 2022
@zadjii-msft zadjii-msft added Help Wanted We encourage anyone to jump in on these. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Apr 26, 2022
@jamespack jamespack mentioned this issue Jul 19, 2023
Merged
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Jul 19, 2023
zadjii-msft pushed a commit that referenced this issue Jul 20, 2023
@jamespack
Replace deprecated await winrt::apartment_context pattern (#15725)
7fba298 
Replace deprecated winrt::apartment_context pattern

I only found 2 instances of this pattern in use and one of them was
actually already replaced but the `co_await winrt::apartment_context`
code was still there.

Tested both window renaming and opening terminal with persisted layouts.
Both still work.

Closes #12982
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Help Wanted We encourage anyone to jump in on these. In-PR This issue has a related PR Issue-Task It's a feature request, but it doesn't really need a major design. Needs-Tag-Fix Doesn't match tag requirements Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
Engineering Improvements
Development

Successfully merging a pull request may close this issue.

Replace deprecated await winrt::apartment_context pattern jamespack/terminal
1 participant
@zadjii-msft