From 6ec2c51f0c30f51c078468d92a1b7d9c478fb2cc Mon Sep 17 00:00:00 2001 From: Brad King Date: Sun, 9 Nov 2025 09:07:01 -0500 Subject: [PATCH] execute_process: Restore support for short timeouts Since commit 5420639a8d (cmExecuteProcessCommand: Replace cmsysProcess with cmUVProcessChain, 2023-06-01, v3.28.0-rc1~138^2~8) we've occasionally observed immediate timeouts on processes that take longer than the timeout to start, even though we only start the timer after the child processes start. The problem is that: * `uv_loop_init` initializes `uv_loop_t`'s cached "now" time. * Starting processes takes time but does not update the "now" time. * `uv_timer_start` computes expiry relative the cached "now" time, so short timers may be expired as soon as they are started. * `uv_run` invokes expired timer callbacks before polling for I/O or process completion, so we "timeout" immediately. Fix this by updating the cached "now" time via `uv_update_time` just before starting timers. This is needed only for timers that start before the `uv_run` event loop. Update our `uv_timer_ptr` wrapper to make all callers consider the choice when calling `start()`. --- Source/CTest/cmCTestBuildHandler.cxx | 2 +- Source/CTest/cmCTestMultiProcessHandler.cxx | 2 +- Source/CTest/cmProcess.cxx | 3 ++- Source/cmCTest.cxx | 3 ++- Source/cmExecuteProcessCommand.cxx | 2 +- Source/cmSystemTools.cxx | 6 ++++-- Source/cmUVHandlePtr.cxx | 6 +++++- Source/cmUVHandlePtr.h | 17 ++++++++++++++++- Source/cmUVProcessChain.cxx | 2 +- Tests/CMakeLib/testUVHandlePtr.cxx | 4 ++-- 10 files changed, 35 insertions(+), 12 deletions(-) diff --git a/Source/CTest/cmCTestBuildHandler.cxx b/Source/CTest/cmCTestBuildHandler.cxx index e962cc7215..ed7481d886 100644 --- a/Source/CTest/cmCTestBuildHandler.cxx +++ b/Source/CTest/cmCTestBuildHandler.cxx @@ -840,7 +840,7 @@ bool cmCTestBuildHandler::RunMakeCommand(const std::string& command, auto* timedOutPtr = static_cast(t->data); *timedOutPtr = true; }, - timeout * 1000, 0); + timeout * 1000, 0, cm::uv_update_time::yes); } // For every chunk of data diff --git a/Source/CTest/cmCTestMultiProcessHandler.cxx b/Source/CTest/cmCTestMultiProcessHandler.cxx index 84ea32b84d..c9dfca697c 100644 --- a/Source/CTest/cmCTestMultiProcessHandler.cxx +++ b/Source/CTest/cmCTestMultiProcessHandler.cxx @@ -733,7 +733,7 @@ void cmCTestMultiProcessHandler::StartNextTestsOnTimer() auto* self = static_cast(timer->data); self->StartNextTests(); }, - milliseconds, 0); + milliseconds, 0, cm::uv_update_time::no); } void cmCTestMultiProcessHandler::FinishTestProcess( diff --git a/Source/CTest/cmProcess.cxx b/Source/CTest/cmProcess.cxx index 2c809b6e97..d42faec88d 100644 --- a/Source/CTest/cmProcess.cxx +++ b/Source/CTest/cmProcess.cxx @@ -156,7 +156,8 @@ void cmProcess::StartTimer() auto msec = std::chrono::duration_cast(*this->Timeout); this->Timer.start(&cmProcess::OnTimeoutCB, - static_cast(msec.count()), 0); + static_cast(msec.count()), 0, + cm::uv_update_time::no); } } diff --git a/Source/cmCTest.cxx b/Source/cmCTest.cxx index 829fbf9238..1bed4f7aae 100644 --- a/Source/cmCTest.cxx +++ b/Source/cmCTest.cxx @@ -3553,7 +3553,8 @@ bool cmCTest::RunCommand(std::vector const& args, auto* timedOutPtr = static_cast(t->data); *timedOutPtr = true; }, - static_cast(timeout.count() * 1000.0), 0); + static_cast(timeout.count() * 1000.0), 0, + cm::uv_update_time::yes); } std::vector tempOutput; diff --git a/Source/cmExecuteProcessCommand.cxx b/Source/cmExecuteProcessCommand.cxx index e186a08d87..9b22d5620d 100644 --- a/Source/cmExecuteProcessCommand.cxx +++ b/Source/cmExecuteProcessCommand.cxx @@ -306,7 +306,7 @@ bool cmExecuteProcessCommand(std::vector const& args, auto* timeoutPtr = static_cast(handle->data); *timeoutPtr = true; }, - timeoutMillis, 0); + timeoutMillis, 0, cm::uv_update_time::yes); } // Read the process output. diff --git a/Source/cmSystemTools.cxx b/Source/cmSystemTools.cxx index 5ad0439c9e..fc958b5aff 100644 --- a/Source/cmSystemTools.cxx +++ b/Source/cmSystemTools.cxx @@ -610,7 +610,8 @@ bool cmSystemTools::RunSingleCommand(std::vector const& command, auto* timedOutPtr = static_cast(t->data); *timedOutPtr = true; }, - static_cast(timeout.count() * 1000.0), 0); + static_cast(timeout.count() * 1000.0), 0, + cm::uv_update_time::yes); } std::vector tempStdOut; @@ -2343,7 +2344,8 @@ cmSystemTools::WaitForLineResult cmSystemTools::WaitForLine( auto* timedOutPtr = static_cast(handle->data); *timedOutPtr = true; }, - static_cast(timeout.count() * 1000.0), 0); + static_cast(timeout.count() * 1000.0), 0, + cm::uv_update_time::no); uv_run(loop, UV_RUN_ONCE); if (timedOut) { diff --git a/Source/cmUVHandlePtr.cxx b/Source/cmUVHandlePtr.cxx index 168f1a6b5c..d757469c80 100644 --- a/Source/cmUVHandlePtr.cxx +++ b/Source/cmUVHandlePtr.cxx @@ -243,9 +243,13 @@ int uv_timer_ptr::init(uv_loop_t& loop, void* data) return uv_timer_init(&loop, *this); } -int uv_timer_ptr::start(uv_timer_cb cb, uint64_t timeout, uint64_t repeat) +int uv_timer_ptr::start(uv_timer_cb cb, uint64_t timeout, uint64_t repeat, + uv_update_time update_time) { assert(this->handle); + if (update_time == uv_update_time::yes) { + ::uv_update_time(this->handle->loop); + } return uv_timer_start(*this, cb, timeout, repeat); } diff --git a/Source/cmUVHandlePtr.h b/Source/cmUVHandlePtr.h index b8b3491977..14b1224a8a 100644 --- a/Source/cmUVHandlePtr.h +++ b/Source/cmUVHandlePtr.h @@ -30,6 +30,20 @@ namespace cm { +/** Whether to call uv_update_time before starting a timer. + * + * uv_loop_t caches a "now" time that uv_loop_init initializes and + * uv_run updates on each event loop iteration. uv_timer_start + * computes timer expiry relative to the loop's cached "now" time. + * For short timeouts started before the event loop, we may need to + * update the "now" time when starting the timer. + */ +enum class uv_update_time +{ + no, + yes, +}; + /*** * RAII class to simplify and ensure the safe usage of uv_loop_t. This includes * making sure resources are properly freed. @@ -251,7 +265,8 @@ struct uv_timer_ptr : public uv_handle_ptr_ int init(uv_loop_t& loop, void* data = nullptr); - int start(uv_timer_cb cb, uint64_t timeout, uint64_t repeat); + int start(uv_timer_cb cb, uint64_t timeout, uint64_t repeat, + uv_update_time update_time); void stop(); }; diff --git a/Source/cmUVProcessChain.cxx b/Source/cmUVProcessChain.cxx index b787f19943..4333e5fb07 100644 --- a/Source/cmUVProcessChain.cxx +++ b/Source/cmUVProcessChain.cxx @@ -439,7 +439,7 @@ bool cmUVProcessChain::Wait(uint64_t milliseconds) auto* timeoutPtr = static_cast(handle->data); *timeoutPtr = true; }, - milliseconds, 0); + milliseconds, 0, cm::uv_update_time::yes); } while (!timeout && diff --git a/Tests/CMakeLib/testUVHandlePtr.cxx b/Tests/CMakeLib/testUVHandlePtr.cxx index 17f672d072..579e52dc1e 100644 --- a/Tests/CMakeLib/testUVHandlePtr.cxx +++ b/Tests/CMakeLib/testUVHandlePtr.cxx @@ -74,7 +74,7 @@ static bool testTimer() cm::uv_timer_ptr timer; timer.init(*loop, &timed); - timer.start(cb, 10, 0); + timer.start(cb, 10, 0, cm::uv_update_time::no); uv_run(loop, UV_RUN_DEFAULT); if (!timed) { @@ -83,7 +83,7 @@ static bool testTimer() } timed = false; - timer.start(cb, 10, 0); + timer.start(cb, 10, 0, cm::uv_update_time::no); timer.stop(); uv_run(loop, UV_RUN_DEFAULT);