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]

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 (1)

src/gateway.cpp:990

• cleanupInternalState() sets connectionStarted = true, which means the flag is never cleared after a successful connect. As a result, ~GatewayImpl() will always call disconnect() at shutdown even if the instance was already disconnected/cleaned up, causing redundant work (and extra logging) during teardown. This flag appears intended to track whether a disconnect is still needed; it should be cleared once internal state is cleaned up.

        {
            std::lock_guard<std::mutex> lock(connectionLog_mtx);
            connectionStarted = true;
        }

[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 (1)

src/gateway.cpp:1011

• cleanupInternalState() sets connectionStarted = true, which makes ~GatewayImpl() believe a connection is still active even after a successful disconnect/cleanup. This can trigger redundant disconnect() calls during teardown (and extra logging / potential NotConnected returns). The flag should be cleared after cleanup so the destructor can skip disconnect when already cleaned up.

        {
            std::lock_guard<std::mutex> lock(connectionLog_mtx);
            connectionStarted = true;
        }

[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)

test/unit/gatewayTest.cpp:1577

• This test asserts disconnect() completes in <200ms, which is likely to be flaky on slower/loaded CI runners and ties the expectation to the default watchdog interval. Make the watchdog cycle explicit (set it to a larger value) and use a looser bound that still detects regressions where disconnect() waits for the full watchdog interval.

    // 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();

[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:989

• cleanupInternalState() holds cleanup_mtx while calling server.stopNotificationWorker(), which joins the notification worker thread. That worker executes user callbacks (see callback.lambda(...) in processQueuedNotifications()), and those callbacks can call back into the gateway (e.g., disconnect()), which will attempt to enter cleanupInternalState() again. This can deadlock (cleanup thread waiting on join while callback thread blocks on cleanup_mtx).

A simple mitigation is to make cleanupInternalState() non-blocking on re-entry by using a try_to_lock and returning if cleanup is already in progress.

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

[Copilot]

Pull request overview

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