From d65f28413895da3cffbe45a4fa73497983cf5f84 Mon Sep 17 00:00:00 2001 From: John Salem Date: Wed, 4 Nov 2020 16:21:59 -0800 Subject: [PATCH] Update IpcStreamFactory state machine to handle being started on a thread that ends (#43711) Handles the case on Windows where Async IO is cancelled due to the thread coreclr started on ending. --- .../src/debug/debug-pal/win/diagnosticsipc.cpp | 108 +++++++++++---------- src/coreclr/src/vm/ipcstreamfactory.cpp | 4 + 2 files changed, 59 insertions(+), 53 deletions(-) diff --git a/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp b/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp index 9a28e48..fe7828d 100644 --- a/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp +++ b/src/coreclr/src/debug/debug-pal/win/diagnosticsipc.cpp @@ -134,6 +134,7 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) _ASSERTE(mode == ConnectionMode::LISTEN); DWORD dwDummy = 0; + IpcStream *pStream = nullptr; bool fSuccess = GetOverlappedResult( _hPipe, // handle &_oOverlap, // overlapped @@ -144,11 +145,14 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) { if (callback != nullptr) callback("Failed to GetOverlappedResults for NamedPipe server", ::GetLastError()); - return nullptr; + // close the pipe (cleaned up and reset below) + ::CloseHandle(_hPipe); + } + else + { + // create new IpcStream using handle (passes ownership to pStream) + pStream = new IpcStream(_hPipe, ConnectionMode::LISTEN); } - - // create new IpcStream using handle and reset the Server object so it can listen again - IpcStream *pStream = new IpcStream(_hPipe, ConnectionMode::LISTEN); // reset the server _hPipe = INVALID_HANDLE_VALUE; @@ -159,7 +163,7 @@ IpcStream *IpcStream::DiagnosticsIpc::Accept(ErrorCallback callback) if (!fSuccess) { delete pStream; - return nullptr; + pStream = nullptr; } return pStream; @@ -434,43 +438,42 @@ bool IpcStream::Read(void *lpBuffer, const uint32_t nBytesToRead, uint32_t &nByt if (!fSuccess) { + DWORD dwError = GetLastError(); + // if we're waiting infinitely, only make one syscall - if (timeoutMs == InfiniteTimeout) + if (timeoutMs == InfiniteTimeout && dwError == ERROR_IO_PENDING) { fSuccess = GetOverlappedResult(_hPipe, // pipe overlap, // overlapped &nNumberOfBytesRead, // out actual number of bytes read true) != 0; // block until async IO completes } - else + else if (dwError == ERROR_IO_PENDING) { - DWORD dwError = GetLastError(); - if (dwError == ERROR_IO_PENDING) + // Wait on overlapped IO event (triggers when async IO is complete regardless of success) + // or timeout + DWORD dwWait = WaitForSingleObject(_oOverlap.hEvent, (DWORD)timeoutMs); + if (dwWait == WAIT_OBJECT_0) { - // Wait on overlapped IO event (triggers when async IO is complete regardless of success) - // or timeout - DWORD dwWait = WaitForSingleObject(_oOverlap.hEvent, (DWORD)timeoutMs); - if (dwWait == WAIT_OBJECT_0) - { - // async IO compelted, get the result - fSuccess = GetOverlappedResult(_hPipe, // pipe - overlap, // overlapped - &nNumberOfBytesRead, // out actual number of bytes read - true) != 0; // block until async IO completes - } - else + // async IO compelted, get the result + fSuccess = GetOverlappedResult(_hPipe, // pipe + overlap, // overlapped + &nNumberOfBytesRead, // out actual number of bytes read + true) != 0; // block until async IO completes + } + else + { + // We either timed out or something else went wrong. + // For any error, attempt to cancel IO and ensure the cancel happened + if (CancelIoEx(_hPipe, overlap) != 0) { - // We either timed out or something else went wrong. - // For any error, attempt to cancel IO and ensure the cancel happened - if (CancelIoEx(_hPipe, overlap) != 0) - { - // check if the async write beat the cancellation - fSuccess = GetOverlappedResult(_hPipe, overlap, &nNumberOfBytesRead, true) != 0; - // Failure here isn't recoverable, so return as such - } + // check if the async write beat the cancellation + fSuccess = GetOverlappedResult(_hPipe, overlap, &nNumberOfBytesRead, true) != 0; + // Failure here isn't recoverable, so return as such } } } + // error is unrecoverable, so return as such } nBytesRead = static_cast(nNumberOfBytesRead); @@ -492,43 +495,42 @@ bool IpcStream::Write(const void *lpBuffer, const uint32_t nBytesToWrite, uint32 if (!fSuccess) { + DWORD dwError = GetLastError(); + // if we're waiting infinitely, only make one syscall - if (timeoutMs == InfiniteTimeout) + if (timeoutMs == InfiniteTimeout && dwError == ERROR_IO_PENDING) { fSuccess = GetOverlappedResult(_hPipe, // pipe overlap, // overlapped &nNumberOfBytesWritten, // out actual number of bytes written true) != 0; // block until async IO completes } - else + else if (dwError == ERROR_IO_PENDING) { - DWORD dwError = GetLastError(); - if (dwError == ERROR_IO_PENDING) + // Wait on overlapped IO event (triggers when async IO is complete regardless of success) + // or timeout + DWORD dwWait = WaitForSingleObject(_oOverlap.hEvent, (DWORD)timeoutMs); + if (dwWait == WAIT_OBJECT_0) { - // Wait on overlapped IO event (triggers when async IO is complete regardless of success) - // or timeout - DWORD dwWait = WaitForSingleObject(_oOverlap.hEvent, (DWORD)timeoutMs); - if (dwWait == WAIT_OBJECT_0) - { - // async IO compelted, get the result - fSuccess = GetOverlappedResult(_hPipe, // pipe - overlap, // overlapped - &nNumberOfBytesWritten, // out actual number of bytes written - true) != 0; // block until async IO completes - } - else + // async IO compelted, get the result + fSuccess = GetOverlappedResult(_hPipe, // pipe + overlap, // overlapped + &nNumberOfBytesWritten, // out actual number of bytes written + true) != 0; // block until async IO completes + } + else + { + // We either timed out or something else went wrong. + // For any error, attempt to cancel IO and ensure the cancel happened + if (CancelIoEx(_hPipe, overlap) != 0) { - // We either timed out or something else went wrong. - // For any error, attempt to cancel IO and ensure the cancel happened - if (CancelIoEx(_hPipe, overlap) != 0) - { - // check if the async write beat the cancellation - fSuccess = GetOverlappedResult(_hPipe, overlap, &nNumberOfBytesWritten, true) != 0; - // Failure here isn't recoverable, so return as such - } + // check if the async write beat the cancellation + fSuccess = GetOverlappedResult(_hPipe, overlap, &nNumberOfBytesWritten, true) != 0; + // Failure here isn't recoverable, so return as such } } } + // error is unrecoverable, so return as such } nBytesWritten = static_cast(nNumberOfBytesWritten); diff --git a/src/coreclr/src/vm/ipcstreamfactory.cpp b/src/coreclr/src/vm/ipcstreamfactory.cpp index 1116f84..dfaf4b9 100644 --- a/src/coreclr/src/vm/ipcstreamfactory.cpp +++ b/src/coreclr/src/vm/ipcstreamfactory.cpp @@ -325,6 +325,10 @@ IpcStream *IpcStreamFactory::GetNextAvailableStream(ErrorCallback callback) if (pStream == nullptr) // only use first signaled stream; will get others on subsequent calls { pStream = ((DiagnosticPort*)(rgIpcPollHandles[i].pUserData))->GetConnectedStream(callback); + if (pStream == nullptr) + { + fSawError = true; + } s_currentPort = (DiagnosticPort*)(rgIpcPollHandles[i].pUserData); } STRESS_LOG2(LF_DIAGNOSTICS_PORT, LL_INFO10, "IpcStreamFactory::GetNextAvailableStream - SIG :: Poll attempt: %d, connection %d signalled.\n", nPollAttempts, i); -- 2.7.4