Fix potential lags/deadlocks during tab close #14041

lhecker · 2022-09-19T23:35:32Z

ConptyClosePseudoConsole blocks until OpenConsole exits.
This is problematic for the changes in 666c446, which stopped calling that
function on a background thread to solve a race condition. This commit fixes
the potential lags/deadlocks from waiting on OpenConsole's exit, by adding
ConptyClosePseudoConsoleNoWait which only closes the IO handles and allows
OpenConsole to exit naturally. This uncovered another potential deadlock
in ServiceLocator::RundownAndExit which might call itself recursively.

Closes #14032

Validation Steps Performed

Print tons of text and concurrently close the tab.
Tab closes, OpenConsole/pwsh exits instantly ✅
Use Enter-VsDevShell and close the tab.
Tab closes instantly, OpenConsole/pwsh exits after ~5 seconds ✅

DHowett · 2022-09-20T16:40:39Z

src/cascadia/TerminalConnection/ConptyConnection.cpp

@@ -631,12 +647,6 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
            const auto result{ til::u8u16(std::string_view{ _buffer.data(), read }, _u16Str, _u8State) };
            if (FAILED(result))
            {
-                if (_isStateAtOrBeyond(ConnectionState::Closing))


Are we certain that we'll never make it here while a connection is closing? As long as we never transition a connection into the Failed state after we transition it into Closing...

That's actually entirely possible and the source of the race condition that I fixed.
While _isStateAtOrBeyond itself might be atomic, that doesn't mean it protects us from other threads changing the state under us concurrently. Unless I misunderstood your question, I don't believe that this is any more or less correct than before. (But the change is required to make the CancelSynchronousIo handling work correctly.)

DHowett · 2022-09-20T16:41:38Z

src/interactivity/base/ServiceLocator.cpp

    // MSFT:40146639
    //   The premise of this function is that 1 thread enters and 0 threads leave alive.
    //   We need to prevent anyone from calling us until we actually ExitProcess(),
    //   so that we don't TriggerTeardown() twice. LockConsole() can't be used here,
    //   because doing so would prevent the render thread from progressing.
-    AcquireSRWLockExclusive(&s_shutdownLock);
+    if (locked.exchange(true, std::memory_order_relaxed))


Is this safe on all architectures? Is relaxed correct?

Yes it's safe on all architectures. Atomic operations themselves are always exactly as atomic as any other, no matter what memory order is specified. Relaxed only means that no additional memory is synchronized/flushed (which is fine here).

src/interactivity/base/ServiceLocator.cpp

DHowett · 2022-09-20T16:44:30Z

src/interactivity/base/ServiceLocator.cpp

-    AcquireSRWLockExclusive(&s_shutdownLock);
+    if (locked.exchange(true, std::memory_order_relaxed))
+    {
+        Sleep(INFINITE);


So just to confirm.

If we get here, it is the first entry and we take the lock, we proceed .We might cause either...

A recursive call to RundownAndExit

Another thread to RundownAndExit

The first one is fixed by preventRecursion, and the second one is fixed by locked -> Sleep(INFINITE)?

What if we trigger another thread to RundownAndExit while we're waiting on it to complete? Is that possible?

No the change to s_shutdownLock is optional. I did it because I initially tried to implement this with a recursive mutex (which SRWLOCK can't do), failed and then wrote the atomic+sleep code, which I kept because I felt like it's easier to see what it does compared to abusing a SRWLOCK for "don't actually lock, only suspend anyone but me".

DHowett · 2022-09-20T16:44:56Z

src/interactivity/base/ServiceLocator.cpp

@@ -39,14 +39,30 @@ void ServiceLocator::SetOneCoreTeardownFunction(void (*pfn)()) noexcept
    s_oneCoreTeardownFunction = pfn;
 }

-[[noreturn]] void ServiceLocator::RundownAndExit(const HRESULT hr)
+void ServiceLocator::RundownAndExit(const HRESULT hr)


Changes here are scary. Are we sure this won't cause the OneCore console to regress?

No I'm not sure, but I don't believe so. The only effective difference is the addition of the thread_local and I could revert the change to s_shutdownLock if you'd like.

DHowett · 2022-09-20T16:52:48Z

What behavior do you see for process management when you open a hundred tabs and then close the window (f.ex)?

lhecker · 2022-09-20T17:39:08Z

What behavior do you see for process management when you open a hundred tabs and then close the window (f.ex)?

Closing ~40 pwsh tabs took about 2-3s for WT to disappear. The pwsh/OpenConsole processes however quit instantly. (Maybe some sort of WinUI lag? Didn't investigate it any further.)
Closing ~20 tabs of the VS dev shell made WT disappear after 1-2s. The pwsh/OpenConsole processes got detached and disappeared after 5-10s (some were faster, some took slightly longer).

DHowett

I'm okay with this. I'll pack it into a 1.16 build and ship it out for folks...!

carlos-zamora

ngl, it's a bit hard to follow. I think with a few answered questions I'll feel comfortable approving though.

carlos-zamora · 2022-09-21T18:42:35Z

src/winconpty/ft_pty/ConPtyTests.cpp

@@ -382,7 +382,7 @@ void ConPtyTests::DiesOnClose()
    Log::Comment(NoThrowString().Format(L"Sleep a bit to let the process attach"));
    Sleep(100);

-    _ClosePseudoConsoleMembers(&pty);
+    _ClosePseudoConsoleMembers(&pty, TRUE);


Y'all don't like default params, huh? :(

It's a C function - that's why it can't have that.

Also FWIW no, I personally don't like default parameters! I also don't like underdocumented boolean parameters for reasons I've mentioned before, but since this is an internal/in-file interface I don't care as much.

carlos-zamora · 2022-09-21T18:45:27Z

src/interactivity/inc/ServiceLocator.hpp

@@ -86,7 +86,6 @@ namespace Microsoft::Console::Interactivity

        static HWND LocatePseudoWindow(const HWND owner = nullptr /*HWND_DESKTOP = 0*/);

-    protected:


Why remove protected? Should we keep protected for some of these?

The class is final so that protected has no effect. It's also not usual / best practice to make classes any more private than necessary. A non-public copy constructor for instance is pretty unusual since you would only be able to use them inside the class. It gets even more unusual when you consider that the functions are explicitly deleted, so not even the class itself can use them. In short, this code was kinda whacky. 😅

carlos-zamora · 2022-09-21T18:51:34Z

src/interactivity/base/ServiceLocator.cpp

-    AcquireSRWLockExclusive(&s_shutdownLock);
+    if (locked.exchange(true, std::memory_order_relaxed))
+    {
+        Sleep(INFINITE);


Please put a big ol' comment here why it's ok to put Sleep(INFINITE);. I think it'd be nice instead of having to find this PR and read through the discussion.

It ended up being a bit of a shorter comment. But I think that one, plus the longer explanation in front of the if condition should sufficiently explain what this does... 🤔

ghost · 2022-09-21T22:25:58Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

`ConptyClosePseudoConsole` blocks until OpenConsole exits. This is problematic for the changes in 666c446, which stopped calling that function on a background thread to solve a race condition. This commit fixes the potential lags/deadlocks from waiting on OpenConsole's exit, by adding `ConptyClosePseudoConsoleNoWait` which only closes the IO handles and allows OpenConsole to exit naturally. This uncovered another potential deadlock in `ServiceLocator::RundownAndExit` which might call itself recursively. Closes #14032 ## Validation Steps Performed * Print tons of text and concurrently close the tab. Tab closes, OpenConsole/pwsh exits instantly ✅ * Use `Enter-VsDevShell` and close the tab. Tab closes instantly, OpenConsole/pwsh exits after ~5 seconds ✅ (cherry picked from commit 274bdb3) Service-Card-Id: 85767341 Service-Version: 1.16

ghost · 2022-09-23T16:56:12Z

🎉Windows Terminal Preview v1.16.2641.0 has been released which incorporates this pull request.:tada:

Handy links:

`ConptyClosePseudoConsole` blocks until OpenConsole exits. This is problematic for the changes in 666c446, which stopped calling that function on a background thread to solve a race condition. This commit fixes the potential lags/deadlocks from waiting on OpenConsole's exit, by adding `ConptyClosePseudoConsoleNoWait` which only closes the IO handles and allows OpenConsole to exit naturally. This uncovered another potential deadlock in `ServiceLocator::RundownAndExit` which might call itself recursively. Closes #14032 ## Validation Steps Performed * Print tons of text and concurrently close the tab. Tab closes, OpenConsole/pwsh exits instantly ✅ * Use `Enter-VsDevShell` and close the tab. Tab closes instantly, OpenConsole/pwsh exits after ~5 seconds ✅ (cherry picked from commit 274bdb3) Service-Card-Id: 86209670 Service-Version: 1.15

ghost · 2022-10-18T21:51:10Z

🎉Windows Terminal v1.15.2874 has been released which incorporates this pull request.:tada:

Handy links:

2 new ConPTY APIs were added as part of this commit: * `ClosePseudoConsoleTimeout` Complements `ClosePseudoConsole`, allowing users to override the `INFINITE` wait for OpenConsole to exit with any arbitrary timeout, including 0. * `ConptyReleasePseudoConsole` This releases the `\Reference` handle held by the `HPCON`. While it makes launching further PTY clients via `PROC_THREAD_ATTRIBUTE_PSEUDOCONSOLE` impossible, it does allow conhost/OpenConsole to exit naturally once all console clients have disconnected. This makes it unnecessary to having to monitor the exit of the spawned shell/application, just to then call `ClosePseudoConsole`, while carefully continuing to read from the output pipe. Instead users can now just read from the output pipe until it is closed, knowing for sure that no more data will come or clients connect. This is especially useful in combination with `ClosePseudoConsoleTimeout` and a timeout of 0, to allow conhost/OpenConsole to exit asynchronously. These new APIs are used to fix an array of bugs around Windows Terminal exiting either too early or too late. They also make usage of the ConPTY API simpler in most situations (when spawning a single application and waiting for it to exit). Depends on #13882, #14041, #14160, #14282 Closes #4564 Closes #14416 Closes MSFT-42244182 ## Validation Steps Performed * Calling `FreeConsole` in a handoff'd application closes the tab ✅ * Create a .bat file containing only `start /B cmd.exe`. If WT stable is the default terminal the tab closes instantly ✅ With these changes included the tab stays open with a cmd.exe prompt ✅ * New ConPTY tests ✅

ghost added the Issue-Bug It either shouldn't be doing this or needs an investigation. label Sep 19, 2022

lhecker force-pushed the dev/lhecker/14032-close-lag branch from ab2abe8 to b8511bd Compare September 20, 2022 01:31

lhecker added Product-Conpty For console issues specifically related to conpty Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Product-Terminal The new Windows Terminal. Priority-1 A description (P1) labels Sep 20, 2022

Fix potential lags/deadlocks during tab close

2f09d15

lhecker force-pushed the dev/lhecker/14032-close-lag branch from b8511bd to 2f09d15 Compare September 20, 2022 14:04

Fix AuditMode errors

8301e45

DHowett reviewed Sep 20, 2022

View reviewed changes

Address feedback

e4bccbf

DHowett approved these changes Sep 20, 2022

View reviewed changes

DHowett requested a review from zadjii-msft September 20, 2022 19:41

carlos-zamora reviewed Sep 21, 2022

View reviewed changes

Address feedback

f142bb9

carlos-zamora approved these changes Sep 21, 2022

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 21, 2022

ghost merged commit 274bdb3 into main Sep 21, 2022

ghost deleted the dev/lhecker/14032-close-lag branch September 21, 2022 22:26

ghost mentioned this pull request Sep 23, 2022

Closing tab hangs when using VS Dev tools and Powershell Core #14032

Closed

lhecker mentioned this pull request Dec 1, 2022

Reported hang when minimized during a razzle build #14134

Closed

lhecker mentioned this pull request Dec 13, 2022

Add support for start /B and FreeConsole #14544

Merged

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix potential lags/deadlocks during tab close #14041

Fix potential lags/deadlocks during tab close #14041

lhecker commented Sep 19, 2022 •

edited

Loading

DHowett Sep 20, 2022

lhecker Sep 20, 2022 •

edited

Loading

DHowett Sep 20, 2022

lhecker Sep 20, 2022

DHowett Sep 20, 2022

lhecker Sep 20, 2022

DHowett Sep 20, 2022

lhecker Sep 20, 2022

DHowett commented Sep 20, 2022

lhecker commented Sep 20, 2022 •

edited

Loading

DHowett left a comment

carlos-zamora left a comment

carlos-zamora Sep 21, 2022

lhecker Sep 21, 2022

DHowett Sep 21, 2022

carlos-zamora Sep 21, 2022

lhecker Sep 21, 2022

carlos-zamora Sep 21, 2022

lhecker Sep 21, 2022

ghost commented Sep 21, 2022

ghost commented Sep 23, 2022

ghost commented Oct 18, 2022

		@@ -86,7 +86,6 @@ namespace Microsoft::Console::Interactivity

		static HWND LocatePseudoWindow(const HWND owner = nullptr /HWND_DESKTOP = 0/);

		protected:

Fix potential lags/deadlocks during tab close #14041

Fix potential lags/deadlocks during tab close #14041

Conversation

lhecker commented Sep 19, 2022 • edited Loading

Validation Steps Performed

Choose a reason for hiding this comment

lhecker Sep 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented Sep 20, 2022

lhecker commented Sep 20, 2022 • edited Loading

DHowett left a comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Sep 21, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Sep 23, 2022

ghost commented Oct 18, 2022

lhecker commented Sep 19, 2022 •

edited

Loading

lhecker Sep 20, 2022 •

edited

Loading

lhecker commented Sep 20, 2022 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.