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

Occasional deadlock in ConptyConnection dtor when a process exits too quickly #7392

Closed
DHowett opened this issue Aug 25, 2020 · 4 comments · Fixed by #7575
Closed

Occasional deadlock in ConptyConnection dtor when a process exits too quickly #7392

DHowett opened this issue Aug 25, 2020 · 4 comments · Fixed by #7575
Labels
Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Milestone
Terminal v1.4

Comments

@DHowett
Copy link
Member

DHowett commented Aug 25, 2020 •
edited
Loading

Seen in the wild. The output thread (last owning reference to ConptyConnection) is tearing down and the ConptyConnection dtor tries to tear down our threadpool wait. The threadpool, however, is waiting for the output thread to drain and complete. Whoops.

Stack traces (partial)

tp teardown

01 0000008a`9b7ff980 00007ff8`d99dd836     KERNELBASE!WaitForSingleObjectEx+0x8e 
02 0000008a`9b7ffa20 00007ff9`52e8aa92     TerminalConnection!winrt::Microsoft::Terminal::TerminalConnection::implementation::ConptyConnection::_ClientTerminated+0xc6 [E:\BA\139\s\src\cascadia\TerminalConnection\ConptyConnection.cpp @ 318]

output thread

06 0000008a`9c54f510 00007ff8`d99dab68     TerminalConnection!wil::details::DestroyThreadPoolWait<0>::Destroy+0x22 [E:\BA\139\s\dep\wil\include\wil\resource.h @ 2375] 
07 (Inline Function) --------`--------     TerminalConnection!wistd::__invoke+0x5 [E:\BA\139\s\dep\wil\include\wil\wistd_type_traits.h @ 4012] 
08 (Inline Function) --------`--------     TerminalConnection!wistd::invoke+0x5 [E:\BA\139\s\dep\wil\include\wil\wistd_functional.h @ 531] 
09 (Inline Function) --------`--------     TerminalConnection!wil::details::resource_policy<_TP_WAIT *,void (__cdecl*)(_TP_WAIT *),&wil::details::DestroyThreadPoolWait<0>::Destroy,wistd::integral_constant<unsigned __int64,0>,_TP_WAIT *,_TP_WAIT *,0,std::nullptr_t>::close+0x5 [E:\BA\139\s\dep\wil\include\wil\resource.h @ 132] 
0a (Inline Function) --------`--------     TerminalConnection!wil::details::unique_storage<wil::details::resource_policy<_TP_WAIT *,void (__cdecl*)(_TP_WAIT *),&wil::details::DestroyThreadPoolWait<0>::Destroy,wistd::integral_constant<unsigned __int64,0>,_TP_WAIT *,_TP_WAIT *,0,std::nullptr_t> >::{dtor}+0x16 [E:\BA\139\s\dep\wil\include\wil\resource.h @ 176] 
0b 0000008a`9c54f540 00007ff8`d99daa54     TerminalConnection!winrt::Microsoft::Terminal::TerminalConnection::implementation::ConptyConnection::~ConptyConnection+0xe8
0c 0000008a`9c54f580 00007ff8`d99b57f0     TerminalConnection!winrt::impl::heap_implements<winrt::Microsoft::Terminal::TerminalConnection::implementation::ConptyConnection>::`scalar deleting destructor'+0x14
0d (Inline Function) --------`--------     TerminalConnection!winrt::impl::root_implements<winrt::Microsoft::Terminal::TerminalConnection::implementation::AzureConnection,winrt::Microsoft::Terminal::TerminalConnection::AzureConnection,winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection>::NonDelegatingRelease+0x76 [E:\BA\139\s\src\cascadia\TerminalConnection\Generated Files\winrt\base.h @ 7058] 
0e (Inline Function) --------`--------     TerminalConnection!winrt::impl::root_implements<winrt::Microsoft::Terminal::TerminalConnection::implementation::AzureConnection,winrt::Microsoft::Terminal::TerminalConnection::AzureConnection,winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection>::Release+0x76 [E:\BA\139\s\src\cascadia\TerminalConnection\Generated Files\winrt\base.h @ 6930] 
0f 0000008a`9c54f5b0 00007ff8`d99dddd5     TerminalConnection!winrt::implements<winrt::Microsoft::Terminal::TerminalConnection::implementation::AzureConnection,winrt::Microsoft::Terminal::TerminalConnection::AzureConnection,winrt::Microsoft::Terminal::TerminalConnection::ITerminalConnection>::Release+0x80 [E:\BA\139\s\src\cascadia\TerminalConnection\Generated Files\winrt\base.h @ 7504] 
10 (Inline Function) --------`--------     TerminalConnection!winrt::com_ptr<winrt::Microsoft::Terminal::TerminalConnection::implementation::ConptyConnection>::release_ref+0x10 [E:\BA\139\s\src\cascadia\TerminalConnection\Generated Files\winrt\base.h @ 2447] 
11 (Inline Function) --------`--------     TerminalConnection!winrt::com_ptr<winrt::Microsoft::Terminal::TerminalConnection::implementation::ConptyConnection>::{dtor}+0x10 [E:\BA\139\s\src\cascadia\TerminalConnection\Generated Files\winrt\base.h @ 2288] 
12 0000008a`9c54f5e0 00007ff8`d99dd50e     TerminalConnection!winrt::Microsoft::Terminal::TerminalConnection::implementation::ConptyConnection::_OutputThread+0x345 [E:\BA\139\s\src\cascadia\TerminalConnection\ConptyConnection.cpp @ 452] 
13 (Inline Function) --------`--------     TerminalConnection!winrt::Microsoft::Terminal::TerminalConnection::implementation::ConptyConnection::Start::__l3::<lambda_223e35fa14b6c169171518cd8f8b0eba>::operator()+0xa [E:\BA\139\s\src\cascadia\TerminalConnection\ConptyConnection.cpp @ 241] 
14 0000008a`9c54f740 00007ff9`518330f4     TerminalConnection!<lambda_223e35fa14b6c169171518cd8f8b0eba>::<lambda_invoker_cdecl>+0xe [E:\BA\139\s\src\cascadia\TerminalConnection\ConptyConnection.cpp @ 244]
@DHowett DHowett added Issue-Bug It either shouldn't be doing this or needs an investigation. Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Severity-Crash Crashes are real bad news. Product-Terminal The new Windows Terminal. Priority-1 A description (P1) labels Aug 25, 2020
@DHowett DHowett added this to the Terminal v1.4 milestone Aug 25, 2020
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 25, 2020
@DHowett DHowett removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Aug 25, 2020
@tamlin-mike
Copy link

QueueUserAPC to a destruction helper?

@DHowett
Copy link
Member Author

DHowett commented Sep 6, 2020

I’m planning on using C++/WinRT’s final release coroutine support :)

DHowett added a commit that referenced this issue Sep 8, 2020
@DHowett
Destruct ConptyConnection on a background thread
a712c3a 
This commit leverages C++/WinRT's final_release [extension point] to
pull the final destruction of ConptyConnection off onto a background
thread.

We've been seeing some deadlocks during teardown where the output thread
(holding the last owning reference to the connection) was trying to
destruct the threadpool wait while the threadpool wait was
simultaneously running its callback and waiting for the output thread to
terminate. It turns out that trying to release a threadpool wait while
it's running a callback that's blocked on you will absolutely result in
a deadlock.

Fixes #7392.

[extension point]: https://devblogs.microsoft.com/oldnewthing/20191018-00
@DHowett DHowett mentioned this issue Sep 8, 2020
Merged
@ghost ghost added the In-PR This issue has a related PR label Sep 8, 2020
@ghost ghost closed this as completed in #7575 Sep 9, 2020
@ghost ghost removed the In-PR This issue has a related PR label Sep 9, 2020
ghost pushed a commit that referenced this issue Sep 9, 2020
@DHowett
Destruct ConptyConnection on a background thread (#7575)
27f7ce7 
This commit leverages C++/WinRT's final_release [extension point] to
pull the final destruction of ConptyConnection off onto a background
thread.

We've been seeing some deadlocks during teardown where the output thread
(holding the last owning reference to the connection) was trying to
destruct the threadpool wait while the threadpool wait was
simultaneously running its callback and waiting for the output thread to
terminate. It turns out that trying to release a threadpool wait while
it's running a callback that's blocked on you will absolutely result in
a deadlock.

Fixes #7392.

[extension point]: https://devblogs.microsoft.com/oldnewthing/20191018-00/?p=103010
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Sep 9, 2020
DHowett added a commit that referenced this issue Sep 18, 2020
@DHowett
Destruct ConptyConnection on a background thread (#7575)
74feda1 
This commit leverages C++/WinRT's final_release [extension point] to
pull the final destruction of ConptyConnection off onto a background
thread.

We've been seeing some deadlocks during teardown where the output thread
(holding the last owning reference to the connection) was trying to
destruct the threadpool wait while the threadpool wait was
simultaneously running its callback and waiting for the output thread to
terminate. It turns out that trying to release a threadpool wait while
it's running a callback that's blocked on you will absolutely result in
a deadlock.

Fixes #7392.

[extension point]: https://devblogs.microsoft.com/oldnewthing/20191018-00/?p=103010

(cherry picked from commit 27f7ce7)
@ghost
Copy link

ghost commented Sep 22, 2020

🎉This issue was addressed in #7575, which has now been successfully released as Windows Terminal v1.3.2651.0.:tada:

Handy links:

@ghost
Copy link

ghost commented Sep 22, 2020

🎉This issue was addressed in #7575, which has now been successfully released as Windows Terminal Preview v1.4.2652.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-TerminalConnection Issues pertaining to the terminal<->backend connection interface Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. Severity-Crash Crashes are real bad news.
Projects
None yet
Milestone
Terminal v1.4
Development

Successfully merging a pull request may close this issue.

Destruct ConptyConnection on a background thread microsoft/terminal
2 participants
@DHowett @tamlin-mike