Fix the static UTF8OutPipeReader & tests (#1998)

This commit addresses some lingering issues in UTF8OutPipeReader and cleans up its termination logic. It also fixes some issues exposed in the test.

Fixes #1997.

src/cascadia/TerminalConnection/ConhostConnection.cpp

@@ -190,14 +190,14 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
 
     DWORD ConhostConnection::_OutputThread()
     {
-        static UTF8OutPipeReader pipeReader{ _outPipe };
+        UTF8OutPipeReader pipeReader{ _outPipe.get() };
         std::string_view strView{};
 
         // process the data of the output pipe in a loop
         while (true)
         {
             HRESULT result = pipeReader.Read(strView);
-            if (FAILED(result))
+            if (FAILED(result) || result == S_FALSE)
             {
                 if (_closing.load())
                 {
@@ -208,7 +208,8 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
                 _disconnectHandlers();
                 return (DWORD)-1;
             }
-            else if (strView.empty())
+
+            if (strView.empty())
             {
                 return 0;
             }

src/types/UTF8OutPipeReader.cpp

@@ -6,11 +6,24 @@
 #include <type_traits>
 #include <utility>
 
-UTF8OutPipeReader::UTF8OutPipeReader(wil::unique_hfile& outPipe) :
+UTF8OutPipeReader::UTF8OutPipeReader(HANDLE outPipe) :
     _outPipe{ outPipe }
 {
 }
 
+// Method Description:
+//   Populates a string_view with *complete* UTF-8 codepoints read from the pipe.
+//   If it receives an incomplete codepoint, it will cache it until it can be completed.
+//   Note: This method trusts that the other end will, in fact, send complete codepoints.
+// Arguments:
+//   - strView: on return, populated with successfully-read codepoints.
+// Return Value:
+//   An HRESULT indicating whether the read was successful. For the purposes of this
+//   method, a closed pipe is considered a successful (but false!) read. All other errors
+//   are translated into an appropriate status code.
+//   S_OK for a successful read
+//   S_FALSE for a read on a closed pipe
+//   E_* (anything) for a failed read
 [[nodiscard]] HRESULT UTF8OutPipeReader::Read(_Out_ std::string_view& strView)
 {
     DWORD dwRead{};
@@ -18,7 +31,7 @@ UTF8OutPipeReader::UTF8OutPipeReader(wil::unique_hfile& outPipe) :
 
     // in case of early escaping
     *_buffer = 0;
-    strView = reinterpret_cast<char*>(_buffer);
+    strView = std::string_view{ reinterpret_cast<char*>(_buffer), 0 };
 
     // copy UTF-8 code units that were remaining from the previously read chunk (if any)
     if (_dwPartialsLen != 0)
@@ -27,19 +40,30 @@ UTF8OutPipeReader::UTF8OutPipeReader(wil::unique_hfile& outPipe) :
     }
 
     // try to read data
-    fSuccess = !!ReadFile(_outPipe.get(), &_buffer[_dwPartialsLen], std::extent<decltype(_buffer)>::value - _dwPartialsLen, &dwRead, nullptr);
+    fSuccess = !!ReadFile(_outPipe, &_buffer[_dwPartialsLen], std::extent<decltype(_buffer)>::value - _dwPartialsLen, &dwRead, nullptr);
 
     dwRead += _dwPartialsLen;
     _dwPartialsLen = 0;
 
+    if (!fSuccess) // reading failed (we must check this first, because dwRead will also be 0.)
+    {
+        auto lastError = GetLastError();
+        if (lastError == ERROR_BROKEN_PIPE)
+        {
+            // This is a successful, but detectable, exit.
+            // There is a chance that we put some partials into the buffer. Since
+            // the pipe has closed, they're just invalid now. They're not worth
+            // reporting.
+            return S_FALSE;
+        }
+
+        return HRESULT_FROM_WIN32(lastError);
+    }
+
     if (dwRead == 0) // quit if no data has been read and no cached data was left over
     {
         return S_OK;
     }
-    else if (!fSuccess) // reading failed
-    {
-        return E_FAIL;
-    }
 
     const BYTE* const endPtr{ _buffer + dwRead };
     const BYTE* backIter{ endPtr - 1 };

src/types/inc/UTF8OutPipeReader.hpp

@@ -27,12 +27,10 @@ Author(s):
 class UTF8OutPipeReader final
 {
 public:
-    UTF8OutPipeReader(wil::unique_hfile& outPipe);
+    UTF8OutPipeReader(HANDLE outPipe);
     [[nodiscard]] HRESULT Read(_Out_ std::string_view& strView);
 
 private:
-    wil::unique_hfile& _outPipe;
-
     enum _Utf8BitMasks : BYTE
     {
         IsAsciiByte = 0b0'0000000, // Any byte representing an ASCII character has the MSB set to 0
@@ -63,6 +61,7 @@ private:
         _Utf8BitMasks::IsLeadByteThreeByteSequence,
     };
 
+    HANDLE _outPipe; // non-owning reference to a pipe.
     BYTE _buffer[4096]{ 0 }; // buffer for the chunk read
     BYTE _utf8Partials[4]{ 0 }; // buffer for code units of a partial UTF-8 code point that have to be cached
     DWORD _dwPartialsLen{}; // number of cached UTF-8 code units

src/types/ut_types/UTF8OutPipeReaderTests.cpp

@@ -109,19 +109,17 @@ class UTF8OutPipeReaderTests
     // Performs the sub-tests.
     HRESULT RunTest(std::string& utf8TestString)
     {
-        HANDLE readFrom{ INVALID_HANDLE_VALUE }, writeTo{ INVALID_HANDLE_VALUE }; // pipe handles
-        SECURITY_ATTRIBUTES sa{ sizeof(SECURITY_ATTRIBUTES) };
-
         std::string_view strView{}; // contains the chunk that we get from UTF8OutPipeReader::Read
         const winrt::hstring utf16Expected{ winrt::to_hstring(utf8TestString) }; // contains the whole string converted to UTF-16
         winrt::hstring utf16Actual{}; // will be concatenated from the converted chunks
 
-        CreatePipe(&readFrom, &writeTo, &sa, 0); // create the pipe handles
+        wil::unique_hfile outPipe{};
+        wil::unique_hfile inPipe{};
 
-        wil::unique_hfile outPipe{ readFrom };
-        wil::unique_hfile inPipe{ writeTo };
+        SECURITY_ATTRIBUTES sa{ sizeof(SECURITY_ATTRIBUTES) };
+        CreatePipe(&outPipe, &inPipe, &sa, 0); // create the pipe handles
 
-        static UTF8OutPipeReader reader{ outPipe }; // declare a static instance of UTF8OutPipeReader
+        UTF8OutPipeReader reader{ outPipe.get() };
 
         ThreadData data{ inPipe, utf8TestString };
 
@@ -132,7 +130,7 @@ class UTF8OutPipeReaderTests
         while (true)
         {
             // get a chunk of UTF-8 data
-            RETURN_IF_FAILED(reader.Read(strView));
+            THROW_IF_FAILED(reader.Read(strView));
 
             if (strView.empty())
             {