Revert "Improve Windows error handling in Diagnostics IPC (#41008)" (#41220)

This reverts commit 5cabbabe3a3bdeaaf8f0b9dc34cf401ee88e86ff.

diff --git a/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp b/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp
index 9af2204..3d3d3f0 100644 (file)
--- a/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp
+++ b/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp
@@ -393,12 +393,12 @@ int32_t IpcStream::DiagnosticsIpc::Poll(IpcPollHandle *rgIpcPollHandles, uint32_
         if (!fSuccess)
         {
             DWORD error = ::GetLastError();
-            if (error == ERROR_PIPE_NOT_CONNECTED || error == ERROR_BROKEN_PIPE)
+            if (error == ERROR_PIPE_NOT_CONNECTED)
                 rgIpcPollHandles[index].revents = (uint8_t)IpcStream::DiagnosticsIpc::PollEvents::HANGUP;
             else
             {
                 if (callback != nullptr)
-                    callback("Client connection error", error);
+                    callback("Client connection error", -1);
                 rgIpcPollHandles[index].revents = (uint8_t)IpcStream::DiagnosticsIpc::PollEvents::ERR;
                 delete[] pHandles;
                 return -1;
@@ -434,56 +434,39 @@ bool IpcStream::Read(void *lpBuffer, const uint32_t nBytesToRead, uint32_t &nByt
 
     if (!fSuccess)
     {
-        DWORD dwReadError = GetLastError();
-        if (dwReadError == ERROR_IO_PENDING)
+        if (timeoutMs == InfiniteTimeout)
         {
-            // we are using Overlapped IO so
-            // this is expected and not an error
-            fSuccess = GetOverlappedResultEx(_hPipe,
-                                            overlap,
-                                            &nNumberOfBytesRead,
-                                            timeoutMs,
-                                            false) != 0;
-            if (!fSuccess)
+            fSuccess = GetOverlappedResult(_hPipe,
+                                           overlap,
+                                           &nNumberOfBytesRead,
+                                           true) != 0;
+        }
+        else
+        {
+            DWORD dwError = GetLastError();
+            if (dwError == ERROR_IO_PENDING)
             {
-                DWORD dwOverlapError = GetLastError();
-                switch (dwOverlapError)
+                DWORD dwWait = WaitForSingleObject(_oOverlap.hEvent, (DWORD)timeoutMs);
+                if (dwWait == WAIT_OBJECT_0)
+                {
+                    // get the result
+                    fSuccess = GetOverlappedResult(_hPipe,
+                                                   overlap,
+                                                   &nNumberOfBytesRead,
+                                                   true) != 0;
+                }
+                else
                 {
-                    case ERROR_IO_INCOMPLETE:
-                        // should only happen if timeout is 0
-                        // this isn't technically an error, but the user requested a 0 timeout and the work hasn't been
-                        // completed yet, so we'll cancel.
-                    case WAIT_TIMEOUT:
-                        // We didn't complete the write in time... cancel the IO
-                        {
-                            fSuccess = CancelIoEx(_hPipe, overlap) != 0;
-                            if (!fSuccess)
-                            {
-                                DWORD dwCancelError = GetLastError();
-                            }
-                            else
-                            {
-                                fSuccess = GetOverlappedResult(_hPipe, overlap, &nNumberOfBytesRead, false) != 0;
-                                // failure either means we successfully cancelled the IO or something else went wrong.
-                                // not worth checking since we can't recover either way.
-                            }
-                        }
-                        break;
-                    case WAIT_IO_COMPLETION:
-                        // We aren't using IO Completion ports so we shouldn't see this...
-                        _ASSERTE(!"IO Completion error when not using IO Completion Ports");
-                    default:
-                        // unrecoverable errors
-                        _ASSERTE(!"IpcStream::Read - Unrecoverable error from GetOverlappedResult");
-                        break;
+                    // cancel IO and ensure the cancel happened
+                    if (CancelIo(_hPipe))
+                    {
+                        // check if the async write beat the cancellation
+                        fSuccess = GetOverlappedResult(_hPipe, overlap, &nNumberOfBytesRead, true) != 0;
+                    }
                 }
             }
         }
-        else
-        {
-            // Other errors are unrecoverable and we should fall through to return failure
-            _ASSERTE(!"IpcStream::Read - Unrecoverable error from ReadFile");
-        }
+        // TODO: Add error handling.
     }
 
     nBytesRead = static_cast<uint32_t>(nNumberOfBytesRead);
@@ -505,56 +488,40 @@ bool IpcStream::Write(const void *lpBuffer, const uint32_t nBytesToWrite, uint32
 
     if (!fSuccess)
     {
-        DWORD dwReadError = GetLastError();
-        if (dwReadError == ERROR_IO_PENDING)
+        DWORD dwError = GetLastError();
+        if (dwError == ERROR_IO_PENDING)
         {
-            // we are using Overlapped IO so
-            // this is expected and not an error
-            fSuccess = GetOverlappedResultEx(_hPipe,
-                                            overlap,
-                                            &nNumberOfBytesWritten,
-                                            timeoutMs,
-                                            false) != 0;
-            if (!fSuccess)
+            if (timeoutMs == InfiniteTimeout)
             {
-                DWORD dwOverlapError = GetLastError();
-                switch (dwOverlapError)
+                // if we're waiting infinitely, don't bother with extra kernel call
+                fSuccess = GetOverlappedResult(_hPipe,
+                                               overlap,
+                                               &nNumberOfBytesWritten,
+                                                true) != 0;
+            }
+            else
+            {
+                DWORD dwWait = WaitForSingleObject(_oOverlap.hEvent, (DWORD)timeoutMs);
+                if (dwWait == WAIT_OBJECT_0)
+                {
+                    // get the result
+                    fSuccess = GetOverlappedResult(_hPipe,
+                                                   overlap,
+                                                   &nNumberOfBytesWritten,
+                                                   true) != 0;
+                }
+                else
                 {
-                    case ERROR_IO_INCOMPLETE:
-                        // should only happen if timeout is 0
-                        // this isn't technically an error, but the user requested a 0 timeout and the work hasn't been
-                        // completed yet, so we'll cancel.
-                    case WAIT_TIMEOUT:
-                        // We didn't complete the write in time... cancel the IO
-                        {
-                            fSuccess = CancelIoEx(_hPipe, overlap) != 0;
-                            if (!fSuccess)
-                            {
-                                DWORD dwCancelError = GetLastError();
-                            }
-                            else
-                            {
-                                fSuccess = GetOverlappedResult(_hPipe, overlap, &nNumberOfBytesWritten, false) != 0;
-                                // failure either means we successfully cancelled the IO or something else went wrong.
-                                // not worth checking since we can't recover either way.
-                            }
-                        }
-                        break;
-                    case WAIT_IO_COMPLETION:
-                        // We aren't using IO Completion ports so we shouldn't see this...
-                        _ASSERTE(!"IO Completion error when not using IO Completion Ports");
-                    default:
-                        // unrecoverable errors
-                        _ASSERTE(!"IpcStream::Write - Unrecoverable error from GetOverlappedResult");
-                        break;
+                    // cancel IO and ensure the cancel happened
+                    if (CancelIo(_hPipe))
+                    {
+                        // check if the async write beat the cancellation
+                        fSuccess = GetOverlappedResult(_hPipe, overlap, &nNumberOfBytesWritten, true) != 0;
+                    }
                 }
             }
         }
-        else
-        {
-            // Other errors are unrecoverable and we should fall through to return failure
-            _ASSERTE(!"IpcStream::Write - Unrecoverable error from WriteFile");
-        }
+        // TODO: Add error handling.
     }
 
     nBytesWritten = static_cast<uint32_t>(nNumberOfBytesWritten);

diff --git a/src/coreclr/src/vm/ipcstreamfactory.cpp b/src/coreclr/src/vm/ipcstreamfactory.cpp
index 1116f84..b6dec28 100644 (file)
--- a/src/coreclr/src/vm/ipcstreamfactory.cpp
+++ b/src/coreclr/src/vm/ipcstreamfactory.cpp
@@ -318,7 +318,7 @@ IpcStream *IpcStreamFactory::GetNextAvailableStream(ErrorCallback callback)
                 {
                     case IpcStream::DiagnosticsIpc::PollEvents::HANGUP:
                         ((DiagnosticPort*)(rgIpcPollHandles[i].pUserData))->Reset(callback);
-                        STRESS_LOG2(LF_DIAGNOSTICS_PORT, LL_INFO10, "IpcStreamFactory::GetNextAvailableStream - HUP :: Poll attempt: %d, connection %d hung up. Connect is reset.\n", nPollAttempts, i);
+                        STRESS_LOG2(LF_DIAGNOSTICS_PORT, LL_INFO10, "IpcStreamFactory::GetNextAvailableStream - HUP :: Poll attempt: %d, connection %d hung up.\n", nPollAttempts, i);
                         pollTimeoutMs = s_pollTimeoutMinMs;
                         break;
                     case IpcStream::DiagnosticsIpc::PollEvents::SIGNALED:
@@ -330,8 +330,7 @@ IpcStream *IpcStreamFactory::GetNextAvailableStream(ErrorCallback callback)
                         STRESS_LOG2(LF_DIAGNOSTICS_PORT, LL_INFO10, "IpcStreamFactory::GetNextAvailableStream - SIG :: Poll attempt: %d, connection %d signalled.\n", nPollAttempts, i);
                         break;
                     case IpcStream::DiagnosticsIpc::PollEvents::ERR:
-                        ((DiagnosticPort*)(rgIpcPollHandles[i].pUserData))->Reset(callback);
-                        STRESS_LOG2(LF_DIAGNOSTICS_PORT, LL_INFO10, "IpcStreamFactory::GetNextAvailableStream - ERR :: Poll attempt: %d, connection %d errored. Connection is reset.\n", nPollAttempts, i);
+                        STRESS_LOG2(LF_DIAGNOSTICS_PORT, LL_INFO10, "IpcStreamFactory::GetNextAvailableStream - ERR :: Poll attempt: %d, connection %d errored.\n", nPollAttempts, i);
                         fSawError = true;
                         break;
                     case IpcStream::DiagnosticsIpc::PollEvents::NONE: