RDKEMW-22043: Upgrade the watchdog logic to use wait_for

[satlead]

No description provided.

[Copilot]

Pull request overview

Upgrades the gateway watchdog thread to use std::condition_variable::wait_for so the watchdog can be woken promptly during disconnect, and adds a unit test to ensure disconnect() is not blocked by the watchdog polling interval.

Changes:

• Replace watchdog sleep_for polling with condition_variable::wait_for plus notification on shutdown.
• Add a timing-based unit test to verify disconnect() completes quickly even with in-flight requests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/gateway.cpp	Implements condition-variable-based watchdog wait and notifies it during disconnect() to avoid waiting for the full interval.
test/unit/gatewayTest.cpp	Adds a regression/performance test asserting disconnect() completes significantly faster than the watchdog interval.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/gateway.cpp:524

• ~GatewayImpl() holds connectionLog_mtx while calling disconnect(). disconnect() joins the transport connection thread; if that thread is concurrently running an onConnectionChange() callback (which also locks connectionLog_mtx), this can deadlock (connection thread blocks on the mutex while disconnect() blocks on join). Also, gating teardown on lastConnectionState risks leaving watchdogThread joinable when the connection never reached the "connected" state, which can trigger std::terminate during destruction.

Prefer calling disconnect() unconditionally without holding connectionLog_mtx (it is already idempotent).

        std::lock_guard<std::mutex> lock(connectionLog_mtx);
        if (lastConnectionState)
        {
            disconnect();
        }

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

src/gateway.cpp:525

• GatewayImpl destructor holds connectionLog_mtx while calling disconnect(). transport.disconnect() can trigger Transport::onClose/onFail which synchronously calls GatewayImpl::onConnectionChange(), and that tries to lock connectionLog_mtx — this can deadlock while disconnect() is waiting for the transport connection thread to join. Only hold connectionLog_mtx long enough to read lastConnectionState, then call disconnect() without the mutex held.

    ~GatewayImpl()
    {
        std::lock_guard<std::mutex> lock(connectionLog_mtx);
        if (lastConnectionState)
        {
            disconnect();
        }
    }

src/gateway.cpp:496

• New private members in GatewayImpl use non-trailing-underscore names (watchdogCv/watchdogMtx). The repo’s “adopt going forward” convention is to add trailing underscores for new private data members when editing these classes, to converge on a consistent style.

    std::thread watchdogThread;
    std::atomic<bool> watchdogRunning;
    std::condition_variable watchdogCv;
    std::mutex watchdogMtx;

test/unit/gatewayTest.cpp:1537

• This test asserts disconnect() completes in <200ms, which is likely to be flaky on loaded CI hosts (thread joins and scheduling jitter can exceed 200ms even if disconnect does not wait for the watchdog interval). Consider making the watchdog interval explicit and much larger (e.g., 5000ms), then asserting disconnect() completes well below that (e.g., <1000ms) to validate the behavior with more margin.

    IGateway& gateway = connectAndWait();

    // Fire a request that the server will never answer.
    auto responseFuture = gateway.request("test.neverResponds", nlohmann::json{});

    // The request is now in-flight and the future is pending. Disconnect and
    // measure the time it takes. With the condition_variable::wait_for refactoring,
    // disconnect() should complete immediately (< 100ms) rather than waiting
    // for the full watchdog interval (500ms).
    auto t0 = std::chrono::steady_clock::now();
    Firebolt::Error disconnectErr = gateway.disconnect();
    auto t1 = std::chrono::steady_clock::now();
    auto disconnectDuration = std::chrono::duration_cast<std::chrono::milliseconds>(t1 - t0).count();

    EXPECT_EQ(disconnectErr, Firebolt::Error::None);

    // Disconnect should complete well within the watchdog interval (500ms).
    // Allow some overhead but it should be significantly faster than 500ms.
    EXPECT_LT(disconnectDuration, 200) << "disconnect() took " << disconnectDuration
                                       << "ms, expected < 200ms (watchdog interval is 500ms)";

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (3)

test/unit/gatewayTest.cpp:1576

• This test assumes the request is still in-flight when measuring disconnect() latency, but Client::request() can resolve the future immediately on a send failure. In that case the test can pass without exercising the intended “disconnect cancels a pending request quickly” behavior. Add a precondition and skip if the request completes early.

    // Fire a request that the server will never answer.
    auto responseFuture = gateway.request("test.neverResponds", nlohmann::json{});

src/gateway.cpp:992

• cleanupInternalState() is declared in the public: section of GatewayImpl. Even though GatewayImpl is internal to this translation unit, keeping teardown helpers private prevents accidental use and better documents intent.

    void cleanupInternalState()
    {
        std::lock_guard<std::mutex> lock(cleanup_mtx);
        if (watchdogRunning.exchange(false))

test/unit/gatewayTest.cpp:1625

• Same issue as DisconnectIsNotTimebound: if gateway.request() resolves immediately (send failure), the test can still pass while not validating the watchdog wake-up / cancelAll path after reconnect. Add a precondition and skip when the future is already ready.

    // New watchdog thread (post-reconnect) should still wake up immediately on disconnect.
    m_messageHandler = [](connection_hdl, server::message_ptr) {};
    auto responseFuture = gateway.request("test.neverResponds", nlohmann::json{});

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (4)

Previously missed (1) — in code that hasn't changed since the last review.

test/unit/gatewayTest.cpp:1592

• This assertion hard-codes 200 and repeats 500ms in the failure message, even though this fixture already defines watchdog_interval_ms (and slack). Deriving the threshold/message from those constants makes the test easier to maintain if timing parameters change.

This issue also appears on line 1630 of the same file.

    // Disconnect should complete well within the watchdog interval (500ms).
    // Allow some overhead but it should be significantly faster than 500ms.
    EXPECT_LT(disconnectDuration, 200) << "disconnect() took " << disconnectDuration
                                       << "ms, expected < 200ms (watchdog interval is 500ms)";

test/unit/gatewayTest.cpp:1628

• The intent is to measure how quickly an in-flight request is canceled by disconnect(). There’s no assertion that responseFuture is still pending before starting the timing, so an early failure (e.g., send fails) could make the future ready immediately and the test would pass without exercising the intended path. Add a precondition check that the future is not ready before measuring elapsed time.

    // New watchdog thread (post-reconnect) should still wake up immediately on disconnect.
    m_messageHandler = [](connection_hdl, server::message_ptr) {};
    auto responseFuture = gateway.request("test.neverResponds", nlohmann::json{});

    auto t0 = std::chrono::steady_clock::now();
    EXPECT_EQ(gateway.disconnect(), Firebolt::Error::None);
    auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::steady_clock::now() - t0).count();

test/unit/gatewayTest.cpp:1631

• This assertion also hard-codes the 200ms threshold and 500ms interval in the message, while the fixture already defines watchdog_interval_ms/slack. Deriving the limit from those constants avoids duplicated magic numbers across tests.

    EXPECT_LT(elapsed, 200) << "disconnect after reconnect took " << elapsed
                            << "ms, expected < 200ms (watchdog interval is 500ms)";

test/unit/gatewayTest.cpp:1642

• The comment says this fixture uses a “short watchdogCycle_ms”, but getTestConfig() doesn’t set cfg.watchdogCycle_ms (so it uses the default). Updating the comment avoids misleading future readers about what timing the test is actually relying on.

    IGateway& gateway = connectAndWait(); // short watchdogCycle_ms + waitTime_ms in this fixture's config

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (5)

Previously missed (4) — in code that hasn't changed since the last review.

src/gateway.cpp:542

• GatewayImpl derives from IGateway (which has a virtual destructor), so the derived destructor should be marked override to make the intent explicit and let the compiler catch signature mismatches. This matches the pattern used elsewhere (e.g., src/helpers_impl.h:36).

    ~GatewayImpl()
    {

src/gateway.cpp:949

• Minor grammar: “cleanup” here is a verb; “clean up” reads clearer and avoids ambiguity in this comment.

        // Disconnection can also happen from the server, it's necessary to cleanup if this ever happens.

test/unit/gatewayTest.cpp:1592

• The failure message hard-codes the watchdog interval as “500ms”. Since the fixture already defines watchdog_interval_ms, using it in the message avoids drift if defaults/config change and makes the test intent clearer.

This issue also appears on line 1629 of the same file.

    // Allow some overhead but it should be significantly faster than 500ms.
    EXPECT_LT(disconnectDuration, 200) << "disconnect() took " << disconnectDuration
                                       << "ms, expected < 200ms (watchdog interval is 500ms)";

test/unit/gatewayTest.cpp:1639

• This newly added test is missing the standard header comment block used for the other new tests in this hunk (name/covers/scenario). Adding it helps keep coverage intent discoverable in the file.

TEST_F(GatewayUTest, TimeoutStillDetectedWhileConnected)
{

test/unit/gatewayTest.cpp:1631

• Same as above: the watchdog interval is hard-coded in the failure message even though the fixture defines watchdog_interval_ms. Using the constant keeps the test correct if the interval changes.

    EXPECT_LT(elapsed, 200) << "disconnect after reconnect took " << elapsed
                            << "ms, expected < 200ms (watchdog interval is 500ms)";

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

test/unit/gatewayTest.cpp:1581

• The timing expectation in this test’s comment doesn’t match the actual assertion: the comment says disconnect should complete "(< 100ms)", but the test asserts < 200ms. Align the comment with the assertion to avoid confusion when the test fails.

    // The request is now in-flight and the future is pending. Disconnect and
    // measure the time it takes. With the condition_variable::wait_for refactoring,
    // disconnect() should complete immediately (< 100ms) rather than waiting
    // for the full watchdog interval (500ms).

test/unit/gatewayTest.cpp:1641

• This inline comment says the fixture uses a “short watchdogCycle_ms” configuration, but getTestConfig() doesn’t set watchdogCycle_ms (it relies on the Firebolt::Config default). Updating the comment will prevent misreading the test’s timing assumptions.

    IGateway& gateway = connectAndWait(); // short watchdogCycle_ms + waitTime_ms in this fixture's config

test/unit/gatewayTest.cpp:1638

• The two new tests added above include the standard “Test name / Covers / Scenario type” header block, but this new test does not. Add the header for consistency and easier coverage auditing.

TEST_F(GatewayUTest, TimeoutStillDetectedWhileConnected)

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (1) — in code that hasn't changed since the last review.

test/unit/gatewayTest.cpp:1585

• Avoid auto for *.count() results here — the type is a simple numeric (std::chrono::milliseconds::rep), and using an explicit type keeps it consistent with the repo’s guideline to reserve auto for long template types (time_points/durations) rather than scalar values.

This issue also appears on line 1628 of the same file.

    auto disconnectDuration = std::chrono::duration_cast<std::chrono::milliseconds>(t1 - t0).count();

src/gateway.cpp:545

• ~GatewayImpl() takes connectionLog_mtx to read connectionStarted and then calls disconnect(), which (via cleanupInternalState()) takes cleanup_mtx and later connectionLog_mtx. Elsewhere (connect()/cleanupInternalState()) the lock order is cleanup_mtx → connectionLog_mtx, so this destructor introduces a lock-order inversion that can deadlock if destruction races with an in-flight connect/cleanup path.

Simplest fix: avoid taking connectionLog_mtx in the destructor and just call disconnect() unconditionally (it is already safe when not connected).

    ~GatewayImpl()
    {
        bool needsDisconnection = false;
        {
            std::lock_guard<std::mutex> lock(connectionLog_mtx);

test/unit/gatewayTest.cpp:1628

• Avoid auto for *.count() results here — count() returns a scalar (std::chrono::milliseconds::rep). Using an explicit type improves readability and follows the repo guideline to avoid auto for short named/scalar types.

    auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::steady_clock::now() - t0).count();