Implement a timeout for PSEUDOCONSOLE_INHERIT_CURSOR

src/host/VtInputThread.cpp

@@ -2,17 +2,15 @@
 // Licensed under the MIT license.
 
 #include "precomp.h"
-
 #include "VtInputThread.hpp"
 
+#include "handle.h"
+#include "output.h"
+#include "server.h"
 #include "../interactivity/inc/ServiceLocator.hpp"
-#include "input.h"
-#include "../terminal/parser/InputStateMachineEngine.hpp"
 #include "../terminal/adapter/InteractDispatch.hpp"
-#include "../types/inc/convert.hpp"
-#include "server.h"
-#include "output.h"
-#include "handle.h"
+#include "../terminal/parser/InputStateMachineEngine.hpp"
+#include "../types/inc/utils.hpp"
 
 using namespace Microsoft::Console;
 using namespace Microsoft::Console::Interactivity;
@@ -23,30 +21,18 @@ using namespace Microsoft::Console::VirtualTerminal;
 // - hPipe - a handle to the file representing the read end of the VT pipe.
 // - inheritCursor - a bool indicating if the state machine should expect a
 //      cursor positioning sequence. See MSFT:15681311.
-VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe,
-                             const bool inheritCursor) :
-    _hFile{ std::move(hPipe) },
-    _hThread{},
-    _u8State{},
-    _dwThreadId{ 0 },
-    _pfnSetLookingForDSR{}
+VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe, const bool inheritCursor) :
+    _hFile{ std::move(hPipe) }
 {
     THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE);
 
     auto dispatch = std::make_unique<InteractDispatch>();
-
     auto engine = std::make_unique<InputStateMachineEngine>(std::move(dispatch), inheritCursor);
 
-    auto engineRef = engine.get();
-
-    _pInputStateMachine = std::make_unique<StateMachine>(std::move(engine));
-
     // we need this callback to be able to flush an unknown input sequence to the app
-    auto flushCallback = [capture0 = _pInputStateMachine.get()] { return capture0->FlushToTerminal(); };
-    engineRef->SetFlushToInputQueueCallback(flushCallback);
+    engine->SetFlushToInputQueueCallback([this] { return _pInputStateMachine->FlushToTerminal(); });
 
-    // we need this callback to capture the reply if someone requests a status from the terminal
-    _pfnSetLookingForDSR = [engineRef](auto&& PH1) { engineRef->SetLookingForDSR(std::forward<decltype(PH1)>(PH1)); };
+    _pInputStateMachine = std::make_unique<StateMachine>(std::move(engine));
 }
 
 // Function Description:
@@ -57,79 +43,67 @@ VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe,
 // - The return value of the underlying instance's _InputThread
 DWORD WINAPI VtInputThread::StaticVtInputThreadProc(_In_ LPVOID lpParameter)
 {
-    const auto pInstance = reinterpret_cast<VtInputThread*>(lpParameter);
+    const auto pInstance = static_cast<VtInputThread*>(lpParameter);
     pInstance->_InputThread();
     return S_OK;
 }
 
-// Method Description:
-// - Do a single ReadFile from our pipe, and try and handle it. If handling
-//      failed, throw or log, depending on what the caller wants.
-// Return Value:
-// - true if you should continue reading
-bool VtInputThread::DoReadInput()
-{
-    char buffer[4096];
-    DWORD dwRead = 0;
-    const auto ok = ReadFile(_hFile.get(), buffer, ARRAYSIZE(buffer), &dwRead, nullptr);
-
-    // The ReadFile() documentations calls out that:
-    // > If the lpNumberOfBytesRead parameter is zero when ReadFile returns TRUE on a pipe, the other
-    // > end of the pipe called the WriteFile function with nNumberOfBytesToWrite set to zero.
-    // But I was unable to replicate any such behavior. I'm not sure it's true anymore.
-    //
-    // However, what the documentations fails to mention is that winsock2 (WSA) handles of the \Device\Afd type are
-    // transparently compatible with ReadFile() and the WSARecv() documentations contains this important information:
-    // > For byte streams, zero bytes having been read [..] indicates graceful closure and that no more bytes will ever be read.
-    // In other words, for pipe HANDLE of unknown type you should consider `lpNumberOfBytesRead == 0` as an exit indicator.
-    //
-    // Here, `dwRead == 0` fixes a deadlock when exiting conhost while being in use by WSL whose hypervisor pipes are WSA.
-    if (!ok || dwRead == 0)
-    {
-        return false;
-    }
-
-    // If we hit a parsing error, eat it. It's bad utf-8, we can't do anything with it.
-    if (FAILED_LOG(til::u8u16({ buffer, gsl::narrow_cast<size_t>(dwRead) }, _wstr, _u8State)))
-    {
-        return true;
-    }
-
-    try
-    {
-        // Make sure to call the GLOBAL Lock/Unlock, not the gci's lock/unlock.
-        // Only the global unlock attempts to dispatch ctrl events. If you use the
-        //      gci's unlock, when you press C-c, it won't be dispatched until the
-        //      next console API call. For something like `powershell sleep 60`,
-        //      that won't happen for 60s
-        LockConsole();
-        const auto unlock = wil::scope_exit([&] { UnlockConsole(); });
-
-        _pInputStateMachine->ProcessString(_wstr);
-    }
-    CATCH_LOG();
-
-    return true;
-}
-
-void VtInputThread::SetLookingForDSR(const bool looking) noexcept
-{
-    if (_pfnSetLookingForDSR)
-    {
-        _pfnSetLookingForDSR(looking);
-    }
-}
-
 // Method Description:
 // - The ThreadProc for the VT Input Thread. Reads input from the pipe, and
 //      passes it to _HandleRunInput to be processed by the
 //      InputStateMachineEngine.
 void VtInputThread::_InputThread()
 {
-    while (DoReadInput())
+    const auto cleanup = wil::scope_exit([this]() {
+        ServiceLocator::LocateGlobals().getConsoleInformation().GetVtIo()->CloseInput();
+    });
+
+    char buffer[4096];
+    DWORD dwRead = 0;
+
+    til::u8state u8State;
+    std::wstring wstr;
+
+    for (;;)
     {
+        const auto ok = ReadFile(_hFile.get(), buffer, ARRAYSIZE(buffer), &dwRead, nullptr);
+
+        // The ReadFile() documentations calls out that:
+        // > If the lpNumberOfBytesRead parameter is zero when ReadFile returns TRUE on a pipe, the other
+        // > end of the pipe called the WriteFile function with nNumberOfBytesToWrite set to zero.
+        // But I was unable to replicate any such behavior. I'm not sure it's true anymore.
+        //
+        // However, what the documentations fails to mention is that winsock2 (WSA) handles of the \Device\Afd type are
+        // transparently compatible with ReadFile() and the WSARecv() documentations contains this important information:
+        // > For byte streams, zero bytes having been read [..] indicates graceful closure and that no more bytes will ever be read.
+        // In other words, for pipe HANDLE of unknown type you should consider `lpNumberOfBytesRead == 0` as an exit indicator.
+        //
+        // Here, `dwRead == 0` fixes a deadlock when exiting conhost while being in use by WSL whose hypervisor pipes are WSA.
+        if (!ok || dwRead == 0)
+        {
+            break;
+        }
+
+        // If we hit a parsing error, eat it. It's bad utf-8, we can't do anything with it.
+        if (FAILED_LOG(til::u8u16({ buffer, gsl::narrow_cast<size_t>(dwRead) }, wstr, u8State)))
+        {
+            continue;
+        }
+
+        try
+        {
+            // Make sure to call the GLOBAL Lock/Unlock, not the gci's lock/unlock.
+            // Only the global unlock attempts to dispatch ctrl events. If you use the
+            //      gci's unlock, when you press C-c, it won't be dispatched until the
+            //      next console API call. For something like `powershell sleep 60`,
+            //      that won't happen for 60s
+            LockConsole();
+            const auto unlock = wil::scope_exit([&] { UnlockConsole(); });
+
+            _pInputStateMachine->ProcessString(wstr);
+        }
+        CATCH_LOG();
     }
-    ServiceLocator::LocateGlobals().getConsoleInformation().GetVtIo()->CloseInput();
 }
 
 // Method Description:
@@ -156,3 +130,9 @@ void VtInputThread::_InputThread()
 
     return S_OK;
 }
+
+void VtInputThread::WaitUntilDSR(DWORD timeout) const noexcept
+{
+    const auto& engine = static_cast<InputStateMachineEngine&>(_pInputStateMachine->Engine());
+    engine.WaitUntilDSR(timeout);
+}

src/host/VtInputThread.hpp

@@ -24,21 +24,16 @@ namespace Microsoft::Console
         VtInputThread(_In_ wil::unique_hfile hPipe, const bool inheritCursor);
 
         [[nodiscard]] HRESULT Start();
-        static DWORD WINAPI StaticVtInputThreadProc(_In_ LPVOID lpParameter);
-        bool DoReadInput();
-        void SetLookingForDSR(const bool looking) noexcept;
+        void WaitUntilDSR(DWORD timeout) const noexcept;
 
     private:
+        static DWORD WINAPI StaticVtInputThreadProc(_In_ LPVOID lpParameter);
         void _InputThread();
 
         wil::unique_hfile _hFile;
         wil::unique_handle _hThread;
-        DWORD _dwThreadId;
-
-        std::function<void(bool)> _pfnSetLookingForDSR;
+        DWORD _dwThreadId = 0;
 
         std::unique_ptr<Microsoft::Console::VirtualTerminal::StateMachine> _pInputStateMachine;
-        til::u8state _u8State;
-        std::wstring _wstr;
     };
 }

src/host/VtIo.cpp

@@ -255,19 +255,14 @@ bool VtIo::IsUsingVt() const
     // MSFT: 15813316
     // If the terminal application wants us to inherit the cursor position,
     //  we're going to emit a VT sequence to ask for the cursor position, then
-    //  read input until we get a response. Terminals who request this behavior
-    //  but don't respond will hang.
+    //  wait 3s until we get a response.
     // If we get a response, the InteractDispatch will call SetCursorPosition,
     //      which will call to our VtIo::SetCursorPosition method.
-    // We need both handles for this initialization to work. If we don't have
-    //      both, we'll skip it. They either aren't going to be reading output
-    //      (so they can't get the DSR) or they can't write the response to us.
     if (_lookingForCursorPosition && _pVtRenderEngine && _pVtInputThread)
     {
+        _lookingForCursorPosition = false;
         LOG_IF_FAILED(_pVtRenderEngine->RequestCursor());
-        while (_lookingForCursorPosition && _pVtInputThread->DoReadInput())
-        {
-        }
+        _pVtInputThread->WaitUntilDSR(3000);
     }
 
     // GH#4999 - Send a sequence to the connected terminal to request

src/terminal/parser/InputStateMachineEngine.cpp

@@ -6,8 +6,9 @@
 #include "stateMachine.hpp"
 #include "InputStateMachineEngine.hpp"
 
+#include <til/atomic.h>
+
 #include "../../inc/unicode.hpp"
-#include "ascii.hpp"
 #include "../../interactivity/inc/VtApiRedirection.hpp"
 
 using namespace Microsoft::Console::VirtualTerminal;
@@ -102,14 +103,19 @@ InputStateMachineEngine::InputStateMachineEngine(std::unique_ptr<IInteractDispat
     THROW_HR_IF_NULL(E_INVALIDARG, _pDispatch.get());
 }
 
-bool InputStateMachineEngine::EncounteredWin32InputModeSequence() const noexcept
+void InputStateMachineEngine::WaitUntilDSR(DWORD timeout) const noexcept
 {
-    return _encounteredWin32InputModeSequence;
+    // Technically we should decrement the timeout with each iteration,
+    // but I suspect infinite spurious wake-ups are a theoretical problem.
+    while (_lookingForDSR.load(std::memory_order::relaxed))
+    {
+        til::atomic_wait(_lookingForDSR, true, timeout);
+    }
 }
 
-void InputStateMachineEngine::SetLookingForDSR(const bool looking) noexcept
+bool InputStateMachineEngine::EncounteredWin32InputModeSequence() const noexcept
 {
-    _lookingForDSR = looking;
+    return _encounteredWin32InputModeSequence;
 }
 
 // Method Description:
@@ -408,12 +414,13 @@ bool InputStateMachineEngine::ActionCsiDispatch(const VTID id, const VTParameter
         // The F3 case is special - it shares a code with the DeviceStatusResponse.
         // If we're looking for that response, then do that, and break out.
         // Else, fall though to the _GetCursorKeysModifierState handler.
-        if (_lookingForDSR)
+        if (_lookingForDSR.load(std::memory_order::relaxed))
         {
             success = _pDispatch->MoveCursor(parameters.at(0), parameters.at(1));
             // Right now we're only looking for on initial cursor
             //      position response. After that, only look for F3.
-            _lookingForDSR = false;
+            _lookingForDSR.store(false, std::memory_order::relaxed);
+            til::atomic_notify_all(_lookingForDSR);
             break;
         }
         [[fallthrough]];

src/terminal/parser/InputStateMachineEngine.hpp

@@ -132,8 +132,9 @@ namespace Microsoft::Console::VirtualTerminal
         InputStateMachineEngine(std::unique_ptr<IInteractDispatch> pDispatch,
                                 const bool lookingForDSR);
 
+        void WaitUntilDSR(DWORD timeout) const noexcept;
+
         bool EncounteredWin32InputModeSequence() const noexcept override;
-        void SetLookingForDSR(const bool looking) noexcept;
 
         bool ActionExecute(const wchar_t wch) override;
         bool ActionExecuteFromEscape(const wchar_t wch) override;
@@ -165,7 +166,7 @@ namespace Microsoft::Console::VirtualTerminal
     private:
         const std::unique_ptr<IInteractDispatch> _pDispatch;
         std::function<bool()> _pfnFlushToInputQueue;
-        bool _lookingForDSR;
+        std::atomic<bool> _lookingForDSR{ false };
         bool _encounteredWin32InputModeSequence = false;
         DWORD _mouseButtonState = 0;
         std::chrono::milliseconds _doubleClickTime;