From 672b44e6848beb45c77ddfd65634035e62af561e Mon Sep 17 00:00:00 2001 From: Jeff Ruan Date: Tue, 19 Nov 2024 10:38:25 +1300 Subject: [PATCH] Replace GetOverlappedResultEx --- include/mega/win32/gfx/worker/comms.h | 10 +++- src/gfx/worker/client.cpp | 1 - src/win32/gfx/worker/comms.cpp | 69 +++++++++++++++++++-------- src/win32/gfx/worker/comms_client.cpp | 2 - tools/gfxworker/src/win32/server.cpp | 51 +++++++++----------- tools/gfxworker/src/win32/server.h | 2 +- 6 files changed, 79 insertions(+), 56 deletions(-) diff --git a/include/mega/win32/gfx/worker/comms.h b/include/mega/win32/gfx/worker/comms.h index 148e6d58e6..59e5fad6a0 100644 --- a/include/mega/win32/gfx/worker/comms.h +++ b/include/mega/win32/gfx/worker/comms.h @@ -63,11 +63,17 @@ class WinOverlapped final OVERLAPPED* data(); - bool isValid() const { return mOverlapped.hEvent != NULL; }; + bool isValid() const + { + return mOverlapped.hEvent != NULL; + }; + + // Return an error code and error string on error + std::pair waitForCompletion(DWORD mWaitMs); private: OVERLAPPED mOverlapped; }; } // namespace -} \ No newline at end of file +} diff --git a/src/gfx/worker/client.cpp b/src/gfx/worker/client.cpp index 1548cc4b60..80ecd2a0e2 100644 --- a/src/gfx/worker/client.cpp +++ b/src/gfx/worker/client.cpp @@ -36,7 +36,6 @@ bool GfxClient::runHello(const std::string& text) auto response = sendAndReceive(endpoint.get(), command); if (response) { - LOG_verbose << "GfxClient gets hello response: " << response->Text; return true; } else diff --git a/src/win32/gfx/worker/comms.cpp b/src/win32/gfx/worker/comms.cpp index 4e96403df2..c06565b830 100644 --- a/src/win32/gfx/worker/comms.cpp +++ b/src/win32/gfx/worker/comms.cpp @@ -24,11 +24,10 @@ WinOverlapped::WinOverlapped() { mOverlapped.Offset = 0; mOverlapped.OffsetHigh = 0; - mOverlapped.hEvent = CreateEvent( - NULL, // default security attribute - TRUE, // manual-reset event - TRUE, // initial state = signaled - NULL); // unnamed event object + mOverlapped.hEvent = CreateEvent(NULL, // default security attribute + TRUE, // manual-reset event + FALSE, // initial state = non signaled + NULL); // unnamed event object if (mOverlapped.hEvent == NULL) { @@ -49,6 +48,30 @@ OVERLAPPED* WinOverlapped::data() return &mOverlapped; } +std::pair WinOverlapped::waitForCompletion(DWORD waitMs) +{ + const auto waitResult = WaitForSingleObject(mOverlapped.hEvent, waitMs); + switch (waitResult) + { + case WAIT_OBJECT_0: + return {std::error_code{}, ""}; + case WAIT_TIMEOUT: + return {std::make_error_code(std::errc::timed_out), + "wait timeout: " + std::to_string(waitMs)}; + case WAIT_ABANDONED: + return {std::make_error_code(std::errc::not_connected), "wait abandoned"}; + case WAIT_FAILED: + { + const auto lastError = GetLastError(); + return {std::make_error_code(std::errc::not_connected), + "wait failed error " + std::to_string(lastError) + " " + + mega::winErrorMessage(lastError)}; + } + default: + return {std::make_error_code(std::errc::not_connected), "wait error"}; + } +} + NamedPipe::NamedPipe(NamedPipe&& other) { this->mPipeHandle = other.mPipeHandle; @@ -61,7 +84,6 @@ NamedPipe::~NamedPipe() if (mPipeHandle != INVALID_HANDLE_VALUE) { CloseHandle(mPipeHandle); - LOG_verbose << "endpoint " << mName << "_" << mPipeHandle << " closed"; } } @@ -108,7 +130,7 @@ bool NamedPipe::doOverlappedOperation(std::function op, return false; } - WinOverlapped overlapped; + WinOverlapped overlapped{}; if (!overlapped.isValid()) { return false; @@ -117,34 +139,39 @@ bool NamedPipe::doOverlappedOperation(std::function op, // Call Op. if (op(overlapped.data())) { - return true; + return true; // Completed } - - // Error - auto lastError = GetLastError(); - if (lastError!= ERROR_IO_PENDING) + else if (const auto lastError = GetLastError(); + lastError != ERROR_IO_PENDING) // Fail with other errors { - LOG_err << mName << ": " << opStr << " pipe failed. error=" << lastError << " " << mega::winErrorMessage(lastError); + LOG_err << mName << ": " << opStr << " pipe failed. error=" << lastError << " " + << mega::winErrorMessage(lastError); return false; } - // Wait op to complete. Negative timeout is infinite + // Wait DWORD waitTimeout = timeout.count() < 0 ? INFINITE : static_cast(timeout.count()); + if (const auto& [error, errorText] = overlapped.waitForCompletion(waitTimeout); error) + { + LOG_verbose << mName << ": " << opStr << " " << errorText; + return false; + } + + // Get result DWORD numberOfBytesTransferred = 0; - bool success = GetOverlappedResultEx(mPipeHandle, - overlapped.data(), - &numberOfBytesTransferred, - waitTimeout, - false); + bool success = GetOverlappedResult(mPipeHandle, + overlapped.data(), + &numberOfBytesTransferred, + false /*bWait*/); if (!success) { - lastError = GetLastError(); + const auto lastError = GetLastError(); LOG_err << mName << ": " << opStr << " pipe fail to complete error=" << lastError << " " << mega::winErrorMessage(lastError); return false; } - // IO completed + // Completed return true; } diff --git a/src/win32/gfx/worker/comms_client.cpp b/src/win32/gfx/worker/comms_client.cpp index eb3289a509..c495c2c3bc 100644 --- a/src/win32/gfx/worker/comms_client.cpp +++ b/src/win32/gfx/worker/comms_client.cpp @@ -61,8 +61,6 @@ std::pair GfxCommunicationsClient::doConnect(LPCTSTR pipeName } } - LOG_verbose << "Connected Handle:" << hPipe << " error: " << static_cast(error); - return {error, hPipe}; } diff --git a/tools/gfxworker/src/win32/server.cpp b/tools/gfxworker/src/win32/server.cpp index c68033895a..1f49194fd7 100644 --- a/tools/gfxworker/src/win32/server.cpp +++ b/tools/gfxworker/src/win32/server.cpp @@ -13,9 +13,7 @@ ServerNamedPipe::~ServerNamedPipe() { if (isValid()) { - LOG_verbose << mName << "Endpoint server flush"; FlushFileBuffers(mPipeHandle); - LOG_verbose << mName << "Endpoint server disconnect"; DisconnectNamedPipe(mPipeHandle); } } @@ -25,58 +23,53 @@ void ServerWin32::operator()() serverListeningLoop(); } -std::error_code ServerWin32::waitForClient(HANDLE hPipe, OVERLAPPED* overlapped) +std::error_code ServerWin32::waitForClient(HANDLE hPipe, WinOverlapped& overlapped) { assert(hPipe != INVALID_HANDLE_VALUE); - assert(overlapped); + assert(overlapped.isValid()); // Wait for the client to connect asynchronous; if it succeeds, // the function returns a nonzero value. // If the function returns zero, - // GetLastError returns ERROR_IO_PENDING, the IO is connected - // GetLastError returns ERROR_PIPE_CONNECTED, the IO is pending - bool success = ConnectNamedPipe(hPipe, overlapped); + // GetLastError returns ERROR_PIPE_CONNECTED, the IO is connected + // GetLastError returns ERROR_IO_PENDING, the IO is pending + bool success = ConnectNamedPipe(hPipe, overlapped.data()); if (success) { LOG_verbose << "Client connected"; return OK; } - if (!success && GetLastError() == ERROR_PIPE_CONNECTED) + if (GetLastError() == ERROR_PIPE_CONNECTED) { LOG_verbose << "Client connected"; return OK; } - if (!success && GetLastError() != ERROR_IO_PENDING) + if (GetLastError() != ERROR_IO_PENDING) { LOG_verbose << "Client couldn't connect, error=" << GetLastError() << " " << mega::winErrorMessage(GetLastError()); return std::make_error_code(std::errc::not_connected); } - // IO_PENDING + // Wait + if (auto [error, errorText] = overlapped.waitForCompletion(mWaitMs); error) + { + LOG_verbose << "Client " << errorText; + return error; + } + + // Get result DWORD numberOfBytesTransferred = 0; - if (GetOverlappedResultEx( - hPipe, - overlapped, - &numberOfBytesTransferred, - mWaitMs, - false)) + if (GetOverlappedResult(hPipe, overlapped.data(), &numberOfBytesTransferred, false /*bWait*/)) { LOG_verbose << "Client connected"; return OK; } - if (GetLastError() == WAIT_TIMEOUT) - { - LOG_verbose << "Wait client connecting Timeout"; - return std::make_error_code(std::errc::timed_out); - } - else - { - LOG_verbose << "Client couldn't connect, error=" << GetLastError() << " " << mega::winErrorMessage(GetLastError()); - return std::make_error_code(std::errc::not_connected); - } + LOG_verbose << "Client couldn't connect, error=" << GetLastError() << " " + << mega::winErrorMessage(GetLastError()); + return std::make_error_code(std::errc::not_connected); } void ServerWin32::serverListeningLoop() @@ -87,6 +80,8 @@ void ServerWin32::serverListeningLoop() return; } + LOG_verbose << "server awaiting client connection"; + const auto fullPipeName = win_utils::toFullPipeName(mPipeName); // first instance to prevent two processes create the same pipe @@ -94,8 +89,6 @@ void ServerWin32::serverListeningLoop() const DWORD BUFSIZE = 512; for (;;) { - LOG_verbose << "server awaiting client connection"; - auto hPipe = CreateNamedPipe( fullPipeName.c_str(), // pipe name PIPE_ACCESS_DUPLEX | // read/write access @@ -120,7 +113,7 @@ void ServerWin32::serverListeningLoop() firstInstance = 0; bool stopRunning = false; - auto err_code = waitForClient(hPipe, overlapped.data()); + auto err_code = waitForClient(hPipe, overlapped); if (err_code) { // if has timeout and expires, we'll stop running diff --git a/tools/gfxworker/src/win32/server.h b/tools/gfxworker/src/win32/server.h index 2a089cbb66..7494707d65 100644 --- a/tools/gfxworker/src/win32/server.h +++ b/tools/gfxworker/src/win32/server.h @@ -49,7 +49,7 @@ class ServerWin32 void serverListeningLoop(); - std::error_code waitForClient(HANDLE hPipe, OVERLAPPED* overlapped); + std::error_code waitForClient(HANDLE hPipe, WinOverlapped& overlapped); std::unique_ptr mRequestProcessor;