OVERLAPPED for stdin, Timeouts 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,26 +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 }
+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();
+    // we need this callback to be able to flush an unknown input sequence to the app
+    engine->SetFlushToInputQueueCallback([this] { return _pInputStateMachine->FlushToTerminal(); });
 
     _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);
 }
 
 // Function Description:
@@ -53,70 +43,94 @@ 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()
+// - 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()
 {
     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;
-    }
+    OVERLAPPED* overlapped = nullptr;
+    OVERLAPPED overlappedBuf{};
+    wil::unique_event overlappedEvent;
+    bool overlappedPending = false;
+    DWORD read = 0;
 
-    try
+    if (Utils::HandleWantsOverlappedIo(_hFile.get()))
     {
-        // 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);
+        overlappedEvent.reset(CreateEventExW(nullptr, nullptr, CREATE_EVENT_MANUAL_RESET, EVENT_ALL_ACCESS));
+        if (overlappedEvent)
+        {
+            overlappedBuf.hEvent = overlappedEvent.get();
+            overlapped = &overlappedBuf;
+        }
     }
-    CATCH_LOG();
-
-    return true;
-}
 
-// 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())
+    for (;;)
     {
+        if (!ReadFile(_hFile.get(), &buffer[0], sizeof(buffer), &read, overlapped))
+        {
+            if (GetLastError() == ERROR_IO_PENDING)
+            {
+                overlappedPending = true;
+            }
+            else
+            {
+                break;
+            }
+        }
+
+        // _wstr can be empty in two situations:
+        // * The previous call to til::u8u16 failed.
+        // * We're using overlapped IO, and it's the first iteration.
+        if (!_wstr.empty())
+        {
+            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();
+        }
+
+        // If we're using overlapped IO, on the first iteration we'll skip the ProcessString()
+        // and call GetOverlappedResult() which blocks until it's done.
+        // On every other iteration we'll call ProcessString() while the IO is already queued up.
+        if (overlappedPending)
+        {
+            overlappedPending = false;
+            if (!GetOverlappedResult(_hFile.get(), overlapped, &read, TRUE))
+            {
+                break;
+            }
+        }
+
+        // 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.
+        // --> Exit if we've read 0 bytes.
+        if (read == 0)
+        {
+            break;
+        }
+
+        // If we hit a parsing error, eat it. It's bad utf-8, we can't do anything with it.
+        FAILED_LOG(til::u8u16({ buffer, gsl::narrow_cast<size_t>(read) }, _wstr, _u8State));
     }
+
     ServiceLocator::LocateGlobals().getConsoleInformation().GetVtIoNoCheck()->CloseInput();
 }
 
@@ -145,8 +159,8 @@ void VtInputThread::_InputThread()
     return S_OK;
 }
 
-bool VtInputThread::IsLookingForDSR() const noexcept
+void VtInputThread::WaitUntilDSR(DWORD timeout) const noexcept
 {
     const auto& engine = static_cast<InputStateMachineEngine&>(_pInputStateMachine->Engine());
-    return engine.IsLookingForDSR();
+    engine.WaitUntilDSR(timeout);
 }

src/host/VtInputThread.hpp

@@ -24,16 +24,15 @@ namespace Microsoft::Console
         VtInputThread(_In_ wil::unique_hfile hPipe, const bool inheritCursor);
 
         [[nodiscard]] HRESULT Start();
-        bool DoReadInput();
-        bool IsLookingForDSR() const 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;
+        DWORD _dwThreadId = 0;
 
         std::unique_ptr<Microsoft::Console::VirtualTerminal::StateMachine> _pInputStateMachine;
         til::u8state _u8State;

src/host/VtIo.cpp

@@ -89,8 +89,8 @@ using namespace Microsoft::Console::Interactivity;
         _overlappedEvent.reset(CreateEventExW(nullptr, nullptr, CREATE_EVENT_MANUAL_RESET, EVENT_ALL_ACCESS));
         if (_overlappedEvent)
         {
-        _overlappedBuf.hEvent = _overlappedEvent.get();
-        _overlapped = &_overlappedBuf;
+            _overlappedBuf.hEvent = _overlappedEvent.get();
+            _overlapped = &_overlappedBuf;
         }
     }
 
@@ -155,40 +155,43 @@ bool VtIo::IsUsingVt() const
         return S_FALSE;
     }
 
-    // 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.
-    // 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 (_pVtInputThread && _pVtInputThread->IsLookingForDSR())
-    {
-        WriteUTF8("\x1b[6n"); // Cursor Position Report (DSR CPR)
-        while (_pVtInputThread->DoReadInput() && _pVtInputThread->IsLookingForDSR())
+    if (_pVtInputThread)
+    {
+        LOG_IF_FAILED(_pVtInputThread->Start());
+    }
+
+    {
+        const auto cork = Cork();
+
+        // GH#4999 - Send a sequence to the connected terminal to request
+        // win32-input-mode from them. This will enable the connected terminal to
+        // send us full INPUT_RECORDs as input. If the terminal doesn't understand
+        // this sequence, it'll just ignore it.
+
+        // By default, DISABLE_NEWLINE_AUTO_RETURN is reset. This implies LNM being set,
+        // which is not the default in terminals, so we have to do that explicitly.
+        WriteUTF8(
+            "\x1b[20h" // Line Feed / New Line Mode (LNM)
+            "\033[?1004h" // Focus Event Mode
+            "\033[?9001h" // Win32 Input Mode
+        );
+
+        // 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
+        //  wait 1s until we get a response.
+        // If we get a response, the InteractDispatch will call SetCursorPosition,
+        //      which will call to our VtIo::SetCursorPosition method.
+        if (_lookingForCursorPosition)
         {
+            WriteUTF8("\x1b[6n"); // Cursor Position Report (DSR CPR)
         }
     }
 
-    // GH#4999 - Send a sequence to the connected terminal to request
-    // win32-input-mode from them. This will enable the connected terminal to
-    // send us full INPUT_RECORDs as input. If the terminal doesn't understand
-    // this sequence, it'll just ignore it.
-
-    // By default, DISABLE_NEWLINE_AUTO_RETURN is reset. This implies LNM being set,
-    // which is not the default in terminals, so we have to do that explicitly.
-    WriteUTF8(
-        "\x1b[20h" // Line Feed / New Line Mode (LNM)
-        "\033[?1004h" // Focus Event Mode
-        "\033[?9001h" // Win32 Input Mode
-    );
-
-    if (_pVtInputThread)
+    if (_lookingForCursorPosition)
     {
-        LOG_IF_FAILED(_pVtInputThread->Start());
+        _lookingForCursorPosition = false;
+        _pVtInputThread->WaitUntilDSR(1000);
     }
 
     if (_pPtySignalInputThread)

src/inc/til/u8u16convert.h

@@ -26,7 +26,7 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
     // state structure for maintenance of UTF-8 partials
     struct u8state
     {
-        char partials[4];
+        char partials[4]{};
         uint8_t have{};
         uint8_t want{};
 

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,9 +103,14 @@ InputStateMachineEngine::InputStateMachineEngine(std::unique_ptr<IInteractDispat
     THROW_HR_IF_NULL(E_INVALIDARG, _pDispatch.get());
 }
 
-bool InputStateMachineEngine::IsLookingForDSR() const noexcept
+void InputStateMachineEngine::WaitUntilDSR(DWORD timeout) const noexcept
 {
-    return _lookingForDSR;
+    // Technically we should decrement the timeout with each iteration,
+    // but I suspect infinite spurious wakeups are a theoretical problem.
+    while (!_lookingForDSR.load(std::memory_order::relaxed))
+    {
+        til::atomic_wait(_lookingForDSR, false, timeout);
+    }
 }
 
 bool InputStateMachineEngine::EncounteredWin32InputModeSequence() const noexcept
@@ -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,7 +132,7 @@ namespace Microsoft::Console::VirtualTerminal
         InputStateMachineEngine(std::unique_ptr<IInteractDispatch> pDispatch,
                                 const bool lookingForDSR);
 
-        bool IsLookingForDSR() const noexcept;
+        void WaitUntilDSR(DWORD timeout) const noexcept;
 
         bool EncounteredWin32InputModeSequence() const noexcept override;
 
@@ -166,7 +166,7 @@ namespace Microsoft::Console::VirtualTerminal
     private:
         const std::unique_ptr<IInteractDispatch> _pDispatch;
         std::function<bool()> _pfnFlushToInputQueue;
-        bool _lookingForDSR;
+        std::atomic<bool> _lookingForDSR;
         bool _encounteredWin32InputModeSequence = false;
         DWORD _mouseButtonState = 0;
         std::chrono::milliseconds _doubleClickTime;