Repository: microsoft/terminal (language: cpp)

Issue:
Closing tab hangs when using VS Dev tools and Powershell Core
### Windows Terminal version

1.16.2524.0

### Windows build number

 10.0.25201.0

### Other Software

* Visual Studio 2022
* Powershell Core 7.2.6

### Steps to reproduce

1. Create a profile using `pwsh.exe` in Windows Terminal Preview
2. Execute the following lines to add the VS dev tools
```powershell
Import-Module "C:\Program Files\Microsoft Visual Studio\2022\Preview\Common7\Tools\Microsoft.VisualStudio.DevShell.dll"
Enter-VsDevShell 8ee891c5 -SkipAutomaticLocation -DevCmdArguments "-arch=x64 -host_arch=x64"
```
3. Close the tab

### Expected Behavior

Tab closes quickly.

### Actual Behavior

wt locks up entirely doing...something. 

![Animation](https://user-images.githubusercontent.com/2447331/190947485-bf79b9ca-82a6-4de5-bb15-7987e7b4dc3d.gif)

I've tested this with the release version of Windows Terminal and also regular powershell.exe. Both of those don't exhibit the behavior. It is only with Powershell Core, the most recent release of wt.exe and the Visual Studio dev tools


Discussion / hints:
Ah, this could (possibly?) be fallout from #13882. /cc @lhecker for when he's got time.
Thanks for the report!
I suspect you are right, I did a few heap dumps to see if I could spot anything of note and I noticed they were both hung on the same line related to the conpty connect, specifically this line https://github.com/microsoft/terminal/blob/main/src/cascadia/TerminalConnection/ConptyConnection.cpp#L557
I debugged this just now and and it's actually hung somewhere else for me:
(It's hung in `WaitForSingleObject(pPty->hConPtyProcess, INFINITE);`)

![image](https://user-images.githubusercontent.com/2256941/191125460-f1f9cf53-ab4d-4845-9959-942837b134d4.png)
Ah right... The old version used to close the connection in the background only, so it didn't matter how long it took and it wasn't visually apparent either: https://github.com/microsoft/terminal/commit/666c446bc38f741a7fc17b481ea1c665a4f31d13#diff-baa2e4fc26af1b8722a4eec13221622ef4ae566e4f7a8658bc58806f96b7e9b8L1525-L1526
But as mentioned in the PR, that's not viable at all, because that's a pretty nasty race condition.
@lhecker, I just double checked - I'm hung on the same line. The one I posted earlier was down further in the stack.  

Fix patch:
diff --git a/src/cascadia/TerminalConnection/ConptyConnection.cpp b/src/cascadia/TerminalConnection/ConptyConnection.cpp
index 6d82a880a2e..69a99a71a1c 100644
--- a/src/cascadia/TerminalConnection/ConptyConnection.cpp
+++ b/src/cascadia/TerminalConnection/ConptyConnection.cpp
@@ -554,6 +554,15 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
 
             _hPC.reset(); // tear down the pseudoconsole (this is like clicking X on a console window)
 
+            // CloseHandle() on pipes blocks until any current WriteFile()/ReadFile() has returned.
+            // CancelSynchronousIo prevents us from deadlocking ourselves.
+            // At this point in Close(), _inPipe won't be used anymore since the UI parts are torn down.
+            // _outPipe is probably still stuck in ReadFile() and might currently be written to.
+            if (_hOutputThread)
+            {
+                CancelSynchronousIo(_hOutputThread.get());
+            }
+
             _inPipe.reset(); // break the pipes
             _outPipe.reset();
 
@@ -615,10 +624,17 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
             DWORD read{};
 
             const auto readFail{ !ReadFile(_outPipe.get(), _buffer.data(), gsl::narrow_cast<DWORD>(_buffer.size()), &read, nullptr) };
+
+            // When we call CancelSynchronousIo() in Close() this is the branch that's taken and gets us out of here.
+            if (_isStateAtOrBeyond(ConnectionState::Closing))
+            {
+                return 0;
+            }
+
             if (readFail) // reading failed (we must check this first, because read will also be 0.)
             {
                 const auto lastError = GetLastError();
-                if (lastError != ERROR_BROKEN_PIPE && !_isStateAtOrBeyond(ConnectionState::Closing))
+                if (lastError != ERROR_BROKEN_PIPE)
                 {
                     // EXIT POINT
                     _indicateExitWithStatus(HRESULT_FROM_WIN32(lastError)); // print a message
@@ -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))
-                {
-                    // This termination was expected.
-                    return 0;
-                }
-
                 // EXIT POINT
                 _indicateExitWithStatus(result); // print a message
                 _transitionToState(ConnectionState::Failed);
diff --git a/src/cascadia/TerminalConnection/ConptyConnection.h b/src/cascadia/TerminalConnection/ConptyConnection.h
index beaaac058f1..90466dfad81 100644
--- a/src/cascadia/TerminalConnection/ConptyConnection.h
+++ b/src/cascadia/TerminalConnection/ConptyConnection.h
@@ -13,7 +13,7 @@
 namespace wil
 {
     // These belong in WIL upstream, so when we reingest the change that has them we'll get rid of ours.
-    using unique_static_pseudoconsole_handle = wil::unique_any<HPCON, decltype(&::ConptyClosePseudoConsole), ::ConptyClosePseudoConsole>;
+    using unique_static_pseudoconsole_handle = wil::unique_any<HPCON, decltype(&::ConptyClosePseudoConsole), ::ConptyClosePseudoConsoleNoWait>;
 }
 
 namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
diff --git a/src/inc/conpty-static.h b/src/inc/conpty-static.h
index 82982864dd6..27fa1437ae6 100644
--- a/src/inc/conpty-static.h
+++ b/src/inc/conpty-static.h
@@ -41,4 +41,6 @@ CONPTY_EXPORT HRESULT WINAPI ConptyReparentPseudoConsole(HPCON hPC, HWND newPare
 
 CONPTY_EXPORT VOID WINAPI ConptyClosePseudoConsole(HPCON hPC);
 
+CONPTY_EXPORT VOID WINAPI ConptyClosePseudoConsoleNoWait(HPCON hPC);
+
 CONPTY_EXPORT HRESULT WINAPI ConptyPackPseudoConsole(HANDLE hServerProcess, HANDLE hRef, HANDLE hSignal, HPCON* phPC);
diff --git a/src/interactivity/base/ServiceLocator.cpp b/src/interactivity/base/ServiceLocator.cpp
index 9471d70b2b9..d21b1be0bc5 100644
--- a/src/interactivity/base/ServiceLocator.cpp
+++ b/src/interactivity/base/ServiceLocator.cpp
@@ -39,14 +39,31 @@ void ServiceLocator::SetOneCoreTeardownFunction(void (*pfn)()) noexcept
     s_oneCoreTeardownFunction = pfn;
 }
 
-[[noreturn]] void ServiceLocator::RundownAndExit(const HRESULT hr)
+void ServiceLocator::RundownAndExit(const HRESULT hr)
 {
+    static thread_local bool preventRecursion = false;
+    static std::atomic<bool> locked;
+
+    // BODGY:
+    // pRender->TriggerTeardown() might cause another VtEngine pass, which then might fail to write to the IO pipe.
+    // If that happens it calls VtIo::CloseOutput(), which in turn calls ServiceLocator::RundownAndExit().
+    // This prevents the unintended recursion and resulting deadlock.
+    if (std::exchange(preventRecursion, true))
+    {
+        return;
+    }
+
     // 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))
+    {
+        // If we reach this point, another thread is already in the process of exiting.
+        // There's a lot of ways to suspend ourselves until we exit, one of which is "sleep forever".
+        Sleep(INFINITE);
+    }
 
     // MSFT:15506250
     // In VT I/O Mode, a client application might die before we've rendered
diff --git a/src/interactivity/inc/ServiceLocator.hpp b/src/interactivity/inc/ServiceLocator.hpp
index 4a5f81b0512..c9221031bbe 100644
--- a/src/interactivity/inc/ServiceLocator.hpp
+++ b/src/interactivity/inc/ServiceLocator.hpp
@@ -32,7 +32,7 @@ namespace Microsoft::Console::Interactivity
     public:
         static void SetOneCoreTeardownFunction(void (*pfn)()) noexcept;
 
-        [[noreturn]] static void RundownAndExit(const HRESULT hr);
+        static void RundownAndExit(const HRESULT hr);
 
         // N.B.: Location methods without corresponding creation methods
         //       automatically create the singleton object on demand.
@@ -86,7 +86,6 @@ namespace Microsoft::Console::Interactivity
 
         static HWND LocatePseudoWindow(const HWND owner = nullptr /*HWND_DESKTOP = 0*/);
 
-    protected:
         ServiceLocator(const ServiceLocator&) = delete;
         ServiceLocator& operator=(const ServiceLocator&) = delete;
 
@@ -112,7 +111,5 @@ namespace Microsoft::Console::Interactivity
         static Globals s_globals;
         static bool s_pseudoWindowInitialized;
         static wil::unique_hwnd s_pseudoWindow;
-
-        static inline SRWLOCK s_shutdownLock = SRWLOCK_INIT;
     };
 }
diff --git a/src/winconpty/dll/winconpty.def b/src/winconpty/dll/winconpty.def
index 1a1952bf421..c5b808e3a96 100644
--- a/src/winconpty/dll/winconpty.def
+++ b/src/winconpty/dll/winconpty.def
@@ -4,6 +4,7 @@ EXPORTS
     ConptyCreatePseudoConsoleAsUser
     ConptyResizePseudoConsole
     ConptyClosePseudoConsole
+    ConptyClosePseudoConsoleNoWait
     ConptyClearPseudoConsole
     ConptyShowHidePseudoConsole
     ConptyReparentPseudoConsole
diff --git a/src/winconpty/ft_pty/ConPtyTests.cpp b/src/winconpty/ft_pty/ConPtyTests.cpp
index 78cfe774f72..92574aba51b 100644
--- a/src/winconpty/ft_pty/ConPtyTests.cpp
+++ b/src/winconpty/ft_pty/ConPtyTests.cpp
@@ -84,10 +84,10 @@ void ConPtyTests::CreateConPtyNoPipes()
     VERIFY_FAILED(_CreatePseudoConsole(defaultSize, nullptr, nullptr, 0, &pcon));
 
     VERIFY_SUCCEEDED(_CreatePseudoConsole(defaultSize, nullptr, goodOut, 0, &pcon));
-    _ClosePseudoConsoleMembers(&pcon);
+    _ClosePseudoConsoleMembers(&pcon, TRUE);
 
     VERIFY_SUCCEEDED(_CreatePseudoConsole(defaultSize, goodIn, nullptr, 0, &pcon));
-    _ClosePseudoConsoleMembers(&pcon);
+    _ClosePseudoConsoleMembers(&pcon, TRUE);
 }
 
 void ConPtyTests::CreateConPtyBadSize()
@@ -131,7 +131,7 @@ void ConPtyTests::GoodCreate()
                              &pcon));
 
     auto closePty = wil::scope_exit([&] {
-        _ClosePseudoConsoleMembers(&pcon);
+        _ClosePseudoConsoleMembers(&pcon, TRUE);
     });
 }
 
@@ -160,7 +160,7 @@ void ConPtyTests::GoodCreateMultiple()
                              0,
                              &pcon1));
     auto closePty1 = wil::scope_exit([&] {
-        _ClosePseudoConsoleMembers(&pcon1);
+        _ClosePseudoConsoleMembers(&pcon1, TRUE);
     });
 
     VERIFY_SUCCEEDED(
@@ -170,7 +170,7 @@ void ConPtyTests::GoodCreateMultiple()
                              0,
                              &pcon2));
     auto closePty2 = wil::scope_exit([&] {
-        _ClosePseudoConsoleMembers(&pcon2);
+        _ClosePseudoConsoleMembers(&pcon2, TRUE);
     });
 }
 
@@ -197,7 +197,7 @@ void ConPtyTests::SurvivesOnBreakInput()
                              0,
                              &pty));
     auto closePty1 = wil::scope_exit([&] {
-        _ClosePseudoConsoleMembers(&pty);
+        _ClosePseudoConsoleMembers(&pty, TRUE);
     });
 
     DWORD dwExit;
@@ -242,7 +242,7 @@ void ConPtyTests::SurvivesOnBreakOutput()
                              0,
                              &pty));
     auto closePty1 = wil::scope_exit([&] {
-        _ClosePseudoConsoleMembers(&pty);
+        _ClosePseudoConsoleMembers(&pty, TRUE);
     });
 
     DWORD dwExit;
@@ -287,7 +287,7 @@ void ConPtyTests::DiesOnBreakBoth()
                              0,
                              &pty));
     auto closePty1 = wil::scope_exit([&] {
-        _ClosePseudoConsoleMembers(&pty);
+        _ClosePseudoConsoleMembers(&pty, TRUE);
     });
 
     DWORD dwExit;
@@ -358,7 +358,7 @@ void ConPtyTests::DiesOnClose()
                              0,
                              &pty));
     auto closePty1 = wil::scope_exit([&] {
-        _ClosePseudoConsoleMembers(&pty);
+        _ClosePseudoConsoleMembers(&pty, TRUE);
     });
 
     DWORD dwExit;
@@ -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);
 
     GetExitCodeProcess(hConPtyProcess.get(), &dwExit);
     VERIFY_ARE_NOT_EQUAL(dwExit, (DWORD)STILL_ACTIVE);
diff --git a/src/winconpty/winconpty.cpp b/src/winconpty/winconpty.cpp
index 19b91b797b3..7a4f9b56395 100644
--- a/src/winconpty/winconpty.cpp
+++ b/src/winconpty/winconpty.cpp
@@ -347,9 +347,10 @@ HRESULT _ReparentPseudoConsole(_In_ const PseudoConsole* const pPty, _In_ const
 //      HPCON via the API).
 // Arguments:
 // - pPty: A pointer to a PseudoConsole struct.
+// - wait: If true, waits for conhost/OpenConsole to exit first.
 // Return Value:
 // - <none>
-void _ClosePseudoConsoleMembers(_In_ PseudoConsole* pPty)
+void _ClosePseudoConsoleMembers(_In_ PseudoConsole* pPty, BOOL wait)
 {
     if (pPty != nullptr)
     {
@@ -365,19 +366,11 @@ void _ClosePseudoConsoleMembers(_In_ PseudoConsole* pPty)
         //      has yet to send before we hard kill it.
         if (_HandleIsValid(pPty->hConPtyProcess))
         {
-            // If the conhost is already dead, then that's fine. Presumably
-            //      it's finished flushing it's output already.
-            DWORD dwExit = 0;
-            // If GetExitCodeProcess failed, it's likely conhost is already dead
-            //      If so, skip waiting regardless of whatever error
-            //      GetExitCodeProcess returned.
-            //      We'll just go straight to killing conhost.
-            if (GetExitCodeProcess(pPty->hConPtyProcess, &dwExit) && dwExit == STILL_ACTIVE)
+            if (wait)
             {
                 WaitForSingleObject(pPty->hConPtyProcess, INFINITE);
             }
 
-            TerminateProcess(pPty->hConPtyProcess, 0);
             CloseHandle(pPty->hConPtyProcess);
             pPty->hConPtyProcess = nullptr;
         }
@@ -398,13 +391,14 @@ void _ClosePseudoConsoleMembers(_In_ PseudoConsole* pPty)
 //      PseudoConsoles that were created with CreatePseudoConsole.
 // Arguments:
 // - pPty: A pointer to a PseudoConsole struct.
+// - wait: If true, waits for conhost/OpenConsole to exit first.
 // Return Value:
 // - <none>
-VOID _ClosePseudoConsole(_In_ PseudoConsole* pPty)
+static void _ClosePseudoConsole(_In_ PseudoConsole* pPty, BOOL wait) noexcept
 {
     if (pPty != nullptr)
     {
-        _ClosePseudoConsoleMembers(pPty);
+        _ClosePseudoConsoleMembers(pPty, wait);
         HeapFree(GetProcessHeap(), 0, pPty);
     }
 }
@@ -465,7 +459,7 @@ extern "C" HRESULT ConptyCreatePseudoConsoleAsUser(_In_ HANDLE hToken,
     auto pPty = (PseudoConsole*)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(PseudoConsole));
     RETURN_IF_NULL_ALLOC(pPty);
     auto cleanupPty = wil::scope_exit([&]() noexcept {
-        _ClosePseudoConsole(pPty);
+        _ClosePseudoConsole(pPty, TRUE);
     });
 
     wil::unique_handle duplicatedInput;
@@ -544,13 +538,22 @@ extern "C" HRESULT WINAPI ConptyReparentPseudoConsole(_In_ HPCON hPC, HWND newPa
 //      console window they were running in was closed.
 // This can fail if the conhost hosting the pseudoconsole failed to be
 //      terminated, or if the pseudoconsole was already terminated.
+// Waits for conhost/OpenConsole to exit first.
 extern "C" VOID WINAPI ConptyClosePseudoConsole(_In_ HPCON hPC)
 {
-    const auto pPty = (PseudoConsole*)hPC;
-    if (pPty != nullptr)
-    {
-        _ClosePseudoConsole(pPty);
-    }
+    _ClosePseudoConsole((PseudoConsole*)hPC, TRUE);
+}
+
+// Function Description:
+// Closes the conpty and all associated state.
+// Client applications attached to the conpty will also behave as though the
+//      console window they were running in was closed.
+// This can fail if the conhost hosting the pseudoconsole failed to be
+//      terminated, or if the pseudoconsole was already terminated.
+// Doesn't wait for conhost/OpenConsole to exit.
+extern "C" VOID WINAPI ConptyClosePseudoConsoleNoWait(_In_ HPCON hPC)
+{
+    _ClosePseudoConsole((PseudoConsole*)hPC, FALSE);
 }
 
 // NOTE: This one is not defined in the Windows headers but is
diff --git a/src/winconpty/winconpty.h b/src/winconpty/winconpty.h
index 289c42e5f28..c52c60e02a9 100644
--- a/src/winconpty/winconpty.h
+++ b/src/winconpty/winconpty.h
@@ -41,8 +41,7 @@ HRESULT _ResizePseudoConsole(_In_ const PseudoConsole* const pPty, _In_ const CO
 HRESULT _ClearPseudoConsole(_In_ const PseudoConsole* const pPty);
 HRESULT _ShowHidePseudoConsole(_In_ const PseudoConsole* const pPty, const bool show);
 HRESULT _ReparentPseudoConsole(_In_ const PseudoConsole* const pPty, _In_ const HWND newParent);
-void _ClosePseudoConsoleMembers(_In_ PseudoConsole* pPty);
-VOID _ClosePseudoConsole(_In_ PseudoConsole* pPty);
+void _ClosePseudoConsoleMembers(_In_ PseudoConsole* pPty, BOOL wait);
 
 HRESULT ConptyCreatePseudoConsoleAsUser(_In_ HANDLE hToken,
                                         _In_ COORD size,


Test patch: