diff --git a/src/host/VtInputThread.cpp b/src/host/VtInputThread.cpp index 495a6e51d8b..4a4b773cb02 100644 --- a/src/host/VtInputThread.cpp +++ b/src/host/VtInputThread.cpp @@ -29,7 +29,6 @@ VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe, _hThread{}, _u8State{}, _dwThreadId{ 0 }, - _exitRequested{ false }, _pfnSetLookingForDSR{} { THROW_HR_IF(E_HANDLE, _hFile.get() == INVALID_HANDLE_VALUE); @@ -50,40 +49,6 @@ VtInputThread::VtInputThread(_In_ wil::unique_hfile hPipe, _pfnSetLookingForDSR = std::bind(&InputStateMachineEngine::SetLookingForDSR, engineRef, std::placeholders::_1); } -// Method Description: -// - Processes a string of input characters. The characters should be UTF-8 -// encoded, and will get converted to wstring to be processed by the -// input state machine. -// Arguments: -// - u8Str - the UTF-8 string received. -// Return Value: -// - S_OK on success, otherwise an appropriate failure. -[[nodiscard]] HRESULT VtInputThread::_HandleRunInput(const std::string_view u8Str) -{ - // 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(); - auto Unlock = wil::scope_exit([&] { UnlockConsole(); }); - - try - { - std::wstring wstr{}; - auto hr = til::u8u16(u8Str, wstr, _u8State); - // If we hit a parsing error, eat it. It's bad utf-8, we can't do anything with it. - if (FAILED(hr)) - { - return S_FALSE; - } - _pInputStateMachine->ProcessString(wstr); - } - CATCH_RETURN(); - - return S_OK; -} - // Function Description: // - Static function used for initializing an instance's ThreadProc. // Arguments: @@ -100,35 +65,50 @@ DWORD WINAPI VtInputThread::StaticVtInputThreadProc(_In_ LPVOID lpParameter) // 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. -// Arguments: -// - throwOnFail: If true, throw an exception if there was an error processing -// the input received. Otherwise, log the error. // Return Value: -// - -void VtInputThread::DoReadInput(const bool throwOnFail) +// - true if you should continue reading +bool VtInputThread::DoReadInput() { char buffer[256]; DWORD dwRead = 0; - auto fSuccess = !!ReadFile(_hFile.get(), buffer, ARRAYSIZE(buffer), &dwRead, nullptr); - - if (!fSuccess) + 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) { - _exitRequested = true; - return; + return false; } - auto hr = _HandleRunInput({ buffer, gsl::narrow_cast(dwRead) }); - if (FAILED(hr)) + try { - if (throwOnFail) - { - _exitRequested = true; - } - else + // 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(); }); + + std::wstring wstr; + // If we hit a parsing error, eat it. It's bad utf-8, we can't do anything with it. + if (SUCCEEDED_LOG(til::u8u16({ buffer, gsl::narrow_cast(dwRead) }, wstr, _u8State))) { - LOG_IF_FAILED(hr); + _pInputStateMachine->ProcessString(wstr); } } + CATCH_LOG(); + + return true; } void VtInputThread::SetLookingForDSR(const bool looking) noexcept @@ -145,9 +125,8 @@ void VtInputThread::SetLookingForDSR(const bool looking) noexcept // InputStateMachineEngine. void VtInputThread::_InputThread() { - while (!_exitRequested) + while (DoReadInput()) { - DoReadInput(true); } ServiceLocator::LocateGlobals().getConsoleInformation().GetVtIo()->CloseInput(); } diff --git a/src/host/VtInputThread.hpp b/src/host/VtInputThread.hpp index 7c075b70025..7652a53887f 100644 --- a/src/host/VtInputThread.hpp +++ b/src/host/VtInputThread.hpp @@ -25,19 +25,16 @@ namespace Microsoft::Console [[nodiscard]] HRESULT Start(); static DWORD WINAPI StaticVtInputThreadProc(_In_ LPVOID lpParameter); - void DoReadInput(const bool throwOnFail); + bool DoReadInput(); void SetLookingForDSR(const bool looking) noexcept; private: - [[nodiscard]] HRESULT _HandleRunInput(const std::string_view u8Str); void _InputThread(); wil::unique_hfile _hFile; wil::unique_handle _hThread; DWORD _dwThreadId; - bool _exitRequested; - std::function _pfnSetLookingForDSR; std::unique_ptr _pInputStateMachine; diff --git a/src/host/VtIo.cpp b/src/host/VtIo.cpp index dd8b635efd9..c286a5e8aca 100644 --- a/src/host/VtIo.cpp +++ b/src/host/VtIo.cpp @@ -282,9 +282,8 @@ bool VtIo::IsUsingVt() const if (_lookingForCursorPosition && _pVtRenderEngine && _pVtInputThread) { LOG_IF_FAILED(_pVtRenderEngine->RequestCursor()); - while (_lookingForCursorPosition) + while (_lookingForCursorPosition && _pVtInputThread->DoReadInput()) { - _pVtInputThread->DoReadInput(false); } }