diff --git a/Source/cmDebuggerPosixPipeConnection.cxx b/Source/cmDebuggerPosixPipeConnection.cxx index ac5fd04828..e4572fa509 100644 --- a/Source/cmDebuggerPosixPipeConnection.cxx +++ b/Source/cmDebuggerPosixPipeConnection.cxx @@ -10,6 +10,7 @@ #include #include +#include namespace cmDebugger { @@ -110,10 +111,13 @@ size_t cmDebuggerPipeConnection_POSIX::read(void* buffer, size_t n) { size_t result = 0; if (rw_pipe >= 0) { - result = ::read(rw_pipe, buffer, n); - if (result == 0) { + ssize_t count = ::read(rw_pipe, buffer, n); + if (count <= 0) { + // EOF or error (including EBADF from a concurrent close). close(); + return 0; } + result = static_cast(count); } return result; @@ -174,17 +178,26 @@ void cmDebuggerPipeClient_POSIX::close() } } +void cmDebuggerPipeClient_POSIX::ShutdownForTesting() +{ + if (isOpen()) { + ::shutdown(rw_pipe, SHUT_RDWR); + } +} + size_t cmDebuggerPipeClient_POSIX::read(void* buffer, size_t n) { - int count = 0; + ssize_t count = 0; if (isOpen()) { - count = static_cast(::read(rw_pipe, buffer, n)); - if (count == 0) { + count = ::read(rw_pipe, buffer, n); + if (count <= 0) { + // EOF or error (including EBADF from a concurrent close). close(); + return 0; } } - return count; + return static_cast(count); } bool cmDebuggerPipeClient_POSIX::write(void const* buffer, size_t n) diff --git a/Source/cmDebuggerPosixPipeConnection.h b/Source/cmDebuggerPosixPipeConnection.h index 98e3f6f264..d1e09d64e2 100644 --- a/Source/cmDebuggerPosixPipeConnection.h +++ b/Source/cmDebuggerPosixPipeConnection.h @@ -69,6 +69,11 @@ public: size_t read(void* buffer, size_t n) override; bool write(void const* buffer, size_t n) override; + // Unit-test helper: signal EOF on both halves of the socket without + // freeing the fd, so a sibling thread blocked in read() wakes up + // cleanly. Production code does not use cmDebuggerPipeClient. + void ShutdownForTesting(); + private: std::string const PipeName; int rw_pipe = -1; diff --git a/Source/cmDebuggerWindowsPipeConnection.h b/Source/cmDebuggerWindowsPipeConnection.h index 5611255a45..dad23fba12 100644 --- a/Source/cmDebuggerWindowsPipeConnection.h +++ b/Source/cmDebuggerWindowsPipeConnection.h @@ -89,6 +89,11 @@ public: size_t read(void* buffer, size_t n) override; bool write(void const* buffer, size_t n) override; + // Unit-test helper: on Windows CloseHandle already cancels any + // pending overlapped I/O, so this just delegates to close(). + // Production code does not use cmDebuggerPipeClient. + void ShutdownForTesting() { this->close(); } + private: std::string GetErrorMessage(DWORD errorCode); diff --git a/Tests/CMakeLib/testDebuggerAdapterPipe.cxx b/Tests/CMakeLib/testDebuggerAdapterPipe.cxx index 92ea047e74..a7af6e8c5f 100644 --- a/Tests/CMakeLib/testDebuggerAdapterPipe.cxx +++ b/Tests/CMakeLib/testDebuggerAdapterPipe.cxx @@ -186,13 +186,24 @@ bool testProtocolWithPipesAbruptDisconnect() std::future adapterFinishedFuture = adapterFinishedPromise.get_future(); - std::promise pipeClosedPromise; - std::future pipeClosedFuture = pipeClosedPromise.get_future(); - std::promise initializedEventReceivedPromise; std::future initializedEventReceivedFuture = initializedEventReceivedPromise.get_future(); + std::promise exitedEventReceivedPromise; + std::future exitedEventReceivedFuture = + exitedEventReceivedPromise.get_future(); + + std::promise terminatedEventReceivedPromise; + std::future terminatedEventReceivedFuture = + terminatedEventReceivedPromise.get_future(); + + std::promise threadStartedPromise; + std::future threadStartedFuture = threadStartedPromise.get_future(); + + std::promise threadExitedPromise; + std::future threadExitedFuture = threadExitedPromise.get_future(); + auto futureTimeout = std::chrono::seconds(60); auto disconnectTimeout = std::chrono::seconds(10); @@ -208,17 +219,26 @@ bool testProtocolWithPipesAbruptDisconnect() client->registerHandler([&](dap::InitializedEvent /*unused*/) { initializedEventReceivedPromise.set_value(true); }); + client->registerHandler([&](dap::ExitedEvent /*unused*/) { + exitedEventReceivedPromise.set_value(true); + }); + client->registerHandler([&](dap::TerminatedEvent const& /*unused*/) { + terminatedEventReceivedPromise.set_value(true); + }); + client->registerHandler([&](dap::ThreadEvent const& e) { + if (e.reason == "started") { + threadStartedPromise.set_value(true); + } else if (e.reason == "exited") { + threadExitedPromise.set_value(true); + } + }); // Raw thread (not ScopedThread): we need to be able to detach on - // failure so the test process can exit even if the bug is present. - // - // Note: we deliberately do NOT call ReportExitCode() here. With the - // bug present, an attempted write to a closed pipe would trigger the - // dap::Session error handler, which masks the busy-loop condition we - // are trying to test for. Instead we let the adapter destructor join - // the SessionThread directly: if SessionThread is spinning on EOF the - // join will hang, the adapterFinishedFuture wait below will time out, - // and the test will fail. + // failure so the test process can exit even if the regression + // returns. With the fix in place, ReportExitCode()'s wait on + // DisconnectEvent unblocks when the SessionThread detects EOF on the + // pipe; without the fix, it blocks forever and the adapter + // destructor hangs in SessionThread.join(). std::thread debuggerThread([&]() { try { auto connection = @@ -228,9 +248,12 @@ bool testProtocolWithPipesAbruptDisconnect() std::shared_ptr debuggerAdapter = std::make_shared( connection, dap::file(stdout, false)); - // Hold the adapter until the test signals that it has closed the - // client side of the pipe. - pipeClosedFuture.wait(); + // Sends thread-exited / exited / terminated events and then + // blocks on DisconnectEvent. The test closes the client side of + // the pipe instead of sending a DisconnectRequest, so the only + // thing that will unblock this wait is the SessionThread's EOF + // handling in cmDebuggerAdapter. + debuggerAdapter->ReportExitCode(0); // Adapter destructed here; joins SessionThread. } catch (std::runtime_error const&) { // Swallowed: connection failures shouldn't hang the test. @@ -249,7 +272,7 @@ bool testProtocolWithPipesAbruptDisconnect() client->bind(client2Debugger, client2Debugger); // Drive the full handshake so that the debugger SessionThread is up - // and blocked reading the pipe. + // and ReportExitCode is blocked on DisconnectEvent. dap::CMakeInitializeRequest initializeRequest; auto initializeResponse = client->send(initializeRequest).get(); ASSERT_TRUE(!initializeResponse.error); @@ -265,20 +288,33 @@ bool testProtocolWithPipesAbruptDisconnect() ASSERT_TRUE(initializedEventReceivedFuture.wait_for(futureTimeout) == std::future_status::ready); + ASSERT_TRUE(terminatedEventReceivedFuture.wait_for(futureTimeout) == + std::future_status::ready); + ASSERT_TRUE(threadStartedFuture.wait_for(futureTimeout) == + std::future_status::ready); + ASSERT_TRUE(threadExitedFuture.wait_for(futureTimeout) == + std::future_status::ready); + ASSERT_TRUE(exitedEventReceivedFuture.wait_for(futureTimeout) == + std::future_status::ready); // Abruptly close the client side without sending DisconnectRequest. // Regression check for the busy-loop bug: the debugger adapter must // detect EOF on the pipe and shut down on its own. - client2Debugger->close(); - pipeClosedPromise.set_value(); + // + // Use ShutdownForTesting() rather than close(). The client dap::Session + // has its own recvThread still blocked in read() on this same socket; + // on Linux, ::close() on an fd does not wake a sibling thread's + // in-flight ::read(), which would deadlock the test. ShutdownForTesting + // calls shutdown(SHUT_RDWR) to signal EOF on the socket endpoint + // without freeing the fd, so both reads wake up naturally. + client2Debugger->ShutdownForTesting(); bool finishedInTime = adapterFinishedFuture.wait_for(disconnectTimeout) == std::future_status::ready; if (!finishedInTime) { - // Bug reproduced: the SessionThread is spinning on EOF and the - // adapter destructor is blocked in SessionThread.join(). Detach so - // the test process can exit instead of hanging in std::thread's - // destructor. + // Bug reproduced: the SessionThread is spinning on EOF and + // ReportExitCode is blocked on DisconnectEvent. Detach so the test + // process can exit instead of hanging in std::thread's destructor. debuggerThread.detach(); ASSERT_TRUE(finishedInTime); }