fix: the watchdog must be interruptible, or stop() waits for its next tick
start_watchdog looped on std::this_thread::sleep_for(watchdog_interval_), and request_stop() cannot wake a sleeping thread. stop_watchdog()'s join therefore blocked until the current sleep expired: three seconds on every teardown at the default interval, and unbounded for anyone who set a long one to keep the periodic report quiet. Now a condition_variable_any waited on with the stop token, so request_stop() ends the wait immediately. Found while writing the next commit's test, which sets a one-hour interval to silence the report and consequently hung for an hour in stop(). The test needs one non-obvious thing, and says so: a pause between start() and stop(). Without it the test races the watchdog — stop_watchdog() runs before the thread has entered its loop, the token is already set when it does, and it exits without ever waiting. That passes against the bug as well as the fix, which is exactly what the first version of this test did. Verified in both directions: without the fix the case is killed at a 25 s timeout; with it, stop() returns in 0 ms.
This commit is contained in:
+15
-2
@@ -8,8 +8,10 @@
|
|||||||
#include <memory>
|
#include <memory>
|
||||||
#endif
|
#endif
|
||||||
|
|
||||||
|
#include <condition_variable>
|
||||||
#include <functional>
|
#include <functional>
|
||||||
#include <iomanip>
|
#include <iomanip>
|
||||||
|
#include <mutex>
|
||||||
#include <iostream>
|
#include <iostream>
|
||||||
#include <map>
|
#include <map>
|
||||||
#include <set>
|
#include <set>
|
||||||
@@ -433,9 +435,20 @@ private:
|
|||||||
|
|
||||||
void start_watchdog() {
|
void start_watchdog() {
|
||||||
watchdog_ = std::jthread([this](std::stop_token tok) {
|
watchdog_ = std::jthread([this](std::stop_token tok) {
|
||||||
|
// Interruptible wait, not sleep_for. request_stop() cannot wake a
|
||||||
|
// sleeping thread, so stop_watchdog()'s join blocked for up to a
|
||||||
|
// full interval — three seconds by default, and unbounded for
|
||||||
|
// anyone who set a long one to keep the periodic report quiet.
|
||||||
|
// Every teardown paid it.
|
||||||
|
std::mutex m;
|
||||||
|
std::condition_variable_any cv;
|
||||||
while (!tok.stop_requested()) {
|
while (!tok.stop_requested()) {
|
||||||
std::this_thread::sleep_for(watchdog_interval_);
|
{
|
||||||
if (tok.stop_requested()) break;
|
std::unique_lock lk(m);
|
||||||
|
if (cv.wait_for(lk, tok, watchdog_interval_,
|
||||||
|
[&tok] { return tok.stop_requested(); }))
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
|
||||||
auto s = collect_snapshots();
|
auto s = collect_snapshots();
|
||||||
check_hung_nodes();
|
check_hung_nodes();
|
||||||
|
|||||||
@@ -1,6 +1,10 @@
|
|||||||
#include <catch2/catch_test_macros.hpp>
|
#include <catch2/catch_test_macros.hpp>
|
||||||
#include <kpn/kpn.hpp>
|
#include <kpn/kpn.hpp>
|
||||||
|
#include <atomic>
|
||||||
#include <chrono>
|
#include <chrono>
|
||||||
|
#include <mutex>
|
||||||
|
#include <stdexcept>
|
||||||
|
#include <string>
|
||||||
#include <thread>
|
#include <thread>
|
||||||
|
|
||||||
using namespace kpn;
|
using namespace kpn;
|
||||||
@@ -54,3 +58,36 @@ TEST_CASE("stop disables input channels — producer push is silently dropped",
|
|||||||
in_ch.push(99);
|
in_ch.push(99);
|
||||||
REQUIRE(in_ch.size() == 0);
|
REQUIRE(in_ch.size() == 0);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Regression: stopping a network must not wait for the watchdog's next tick.
|
||||||
|
//
|
||||||
|
// The watchdog looped on std::this_thread::sleep_for(watchdog_interval_), and
|
||||||
|
// request_stop() cannot wake a sleeping thread — so stop_watchdog()'s join
|
||||||
|
// blocked until the current sleep expired. Every teardown paid up to a full
|
||||||
|
// interval, three seconds by default, and a caller who set a long one to keep
|
||||||
|
// the periodic report quiet got a stop() that looked like a hang. That is how
|
||||||
|
// this was found: the error-handler case above set an hour.
|
||||||
|
TEST_CASE("stopping a network does not wait for the watchdog interval", "[network]") {
|
||||||
|
auto node = kpn::make_node<increment>(kpn::in<"v">{}, kpn::out<"w">{}, 4);
|
||||||
|
kpn::Channel<int> out(4);
|
||||||
|
node.set_output_channel<0>(&out);
|
||||||
|
|
||||||
|
kpn::Network net;
|
||||||
|
net.add("inc", node).build();
|
||||||
|
net.set_watchdog_interval(std::chrono::hours(1));
|
||||||
|
net.start();
|
||||||
|
|
||||||
|
// Let the watchdog actually reach its wait. Without this the test races it:
|
||||||
|
// stop_watchdog() runs before the thread has entered the loop, the token is
|
||||||
|
// already set when it does, and it exits without ever waiting — which passes
|
||||||
|
// against the bug as well as the fix.
|
||||||
|
std::this_thread::sleep_for(std::chrono::milliseconds(100));
|
||||||
|
|
||||||
|
const auto t0 = std::chrono::steady_clock::now();
|
||||||
|
net.stop();
|
||||||
|
const auto ms = std::chrono::duration_cast<std::chrono::milliseconds>(
|
||||||
|
std::chrono::steady_clock::now() - t0).count();
|
||||||
|
|
||||||
|
INFO("stop took " << ms << " ms");
|
||||||
|
CHECK(ms < 2000);
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user