From 8977095f70c9ad64614b87d291d422df38892231 Mon Sep 17 00:00:00 2001 From: Odysseus Date: Wed, 22 Oct 2025 10:50:33 -0700 Subject: [PATCH] Fix the wrong check of running thread --- src/common/service/NetRemoteService.cxx | 28 +++++++- .../net/remote/service/NetRemoteService.hxx | 2 + tests/unit/TestNetRemoteServiceClient.cxx | 69 +++++++++++++++++++ 3 files changed, 97 insertions(+), 2 deletions(-) diff --git a/src/common/service/NetRemoteService.cxx b/src/common/service/NetRemoteService.cxx index 64b19ca7..6727eb7e 100644 --- a/src/common/service/NetRemoteService.cxx +++ b/src/common/service/NetRemoteService.cxx @@ -707,12 +707,20 @@ NetRemoteService::WifiAccessPointTimedEnableImpl(std::string_view accessPointId, // Right now, this service only supports managing singale access point. { std::lock_guard lock(m_threadsMutex); - if (m_timedEnableThread && m_timedEnableThread->joinable()) { + if (m_timedEnableRunning.load()) { wifiOperationStatus.set_code(WifiAccessPointOperationStatusCode::WifiAccessPointOperationStatusCodeOperationNotSupported); wifiOperationStatus.set_message("A timed enable operation is already in progress"); return wifiOperationStatus; } + // Join previous thread if it exists and is joinable + if (m_timedEnableThread && m_timedEnableThread->joinable()) { + m_timedEnableThread->join(); + } + + // Set the running flag before creating the thread + m_timedEnableRunning = true; + // Create and store the timer thread auto timerThread = std::make_shared([this, accessPointId = std::string(accessPointId), hasConfiguration = (dot11AccessPointConfiguration != nullptr), configurationCopy = dot11AccessPointConfiguration ? *dot11AccessPointConfiguration : Dot11AccessPointConfiguration{}, accessPointController, durationSeconds]() { // Sleep for the specified duration, checking every second for shutdown signal @@ -725,6 +733,7 @@ NetRemoteService::WifiAccessPointTimedEnableImpl(std::string_view accessPointId, // If we were shutdown, exit early if (m_shutdown.load()) { LOGI << std::format("Timed enable operation for access point {} was cancelled due to service shutdown", accessPointId); + m_timedEnableRunning = false; return; } @@ -741,6 +750,9 @@ NetRemoteService::WifiAccessPointTimedEnableImpl(std::string_view accessPointId, accessPointId, durationSeconds); } + + // Clear the running flag when the operation completes + m_timedEnableRunning = false; }); m_timedEnableThread = timerThread; @@ -773,12 +785,20 @@ NetRemoteService::WifiAccessPointTimedDisableImpl(std::string_view accessPointId // Right now, this service only supports managing singale access point. { std::lock_guard lock(m_threadsMutex); - if (m_timedDisableThread && m_timedDisableThread->joinable()) { + if (m_timedDisableRunning.load()) { wifiOperationStatus.set_code(WifiAccessPointOperationStatusCode::WifiAccessPointOperationStatusCodeOperationNotSupported); wifiOperationStatus.set_message("A timed disable operation is already in progress"); return wifiOperationStatus; } + // Join previous thread if it exists and is joinable + if (m_timedDisableThread && m_timedDisableThread->joinable()) { + m_timedDisableThread->join(); + } + + // Set the running flag before creating the thread + m_timedDisableRunning = true; + // Create and store the timer thread auto timerThread = std::make_shared([this, accessPointId = std::string(accessPointId), accessPointController, durationSeconds]() { // Sleep for the specified duration, checking every second for shutdown signal @@ -791,6 +811,7 @@ NetRemoteService::WifiAccessPointTimedDisableImpl(std::string_view accessPointId // If we were shutdown, exit early if (m_shutdown.load()) { LOGI << std::format("Timed disable operation for access point {} was cancelled due to service shutdown", accessPointId); + m_timedDisableRunning = false; return; } @@ -806,6 +827,9 @@ NetRemoteService::WifiAccessPointTimedDisableImpl(std::string_view accessPointId accessPointId, durationSeconds); } + + // Clear the running flag when the operation completes + m_timedDisableRunning = false; }); m_timedDisableThread = timerThread; diff --git a/src/common/service/include/microsoft/net/remote/service/NetRemoteService.hxx b/src/common/service/include/microsoft/net/remote/service/NetRemoteService.hxx index 93aaa8b8..5f66c80c 100644 --- a/src/common/service/include/microsoft/net/remote/service/NetRemoteService.hxx +++ b/src/common/service/include/microsoft/net/remote/service/NetRemoteService.hxx @@ -387,6 +387,8 @@ private: // Thread management for timed operations std::mutex m_threadsMutex; std::atomic m_shutdown{ false }; + std::atomic m_timedEnableRunning{ false }; + std::atomic m_timedDisableRunning{ false }; std::shared_ptr m_timedEnableThread; std::shared_ptr m_timedDisableThread; }; diff --git a/tests/unit/TestNetRemoteServiceClient.cxx b/tests/unit/TestNetRemoteServiceClient.cxx index ab2546a5..1dd8968e 100644 --- a/tests/unit/TestNetRemoteServiceClient.cxx +++ b/tests/unit/TestNetRemoteServiceClient.cxx @@ -5,6 +5,7 @@ #include #include #include +#include #include #include @@ -1349,6 +1350,39 @@ TEST_CASE("WifiAccessPointTimedEnable API", "[basic][rpc][client][remote][timed] REQUIRE(result2.status().code() == WifiAccessPointOperationStatusCode::WifiAccessPointOperationStatusCodeOperationNotSupported); REQUIRE(result2.status().message() == "A timed enable operation is already in progress"); } + + SECTION("Succeeds when a timed enable operation is finished") + { + // Start first timed enable operation with short duration + WifiAccessPointTimedEnableRequest request1{}; + request1.set_accesspointid(InterfaceName1); + request1.set_durationseconds(1); // 1 second duration + + WifiAccessPointTimedEnableResult result1{}; + grpc::ClientContext clientContext1{}; + + auto status1 = client->WifiAccessPointTimedEnable(&clientContext1, request1, &result1); + REQUIRE(status1.ok()); + REQUIRE(result1.has_status()); + REQUIRE(result1.status().code() == WifiAccessPointOperationStatusCode::WifiAccessPointOperationStatusCodeSucceeded); + + // Wait for the first operation to complete + std::this_thread::sleep_for(std::chrono::seconds(2)); + + // Start second timed enable operation after the first has finished + WifiAccessPointTimedEnableRequest request2{}; + request2.set_accesspointid(InterfaceName2); + request2.set_durationseconds(1); // 1 second duration + + WifiAccessPointTimedEnableResult result2{}; + grpc::ClientContext clientContext2{}; + + auto status2 = client->WifiAccessPointTimedEnable(&clientContext2, request2, &result2); + REQUIRE(status2.ok()); + REQUIRE(result2.has_status()); + REQUIRE(result2.status().code() == WifiAccessPointOperationStatusCode::WifiAccessPointOperationStatusCodeSucceeded); + REQUIRE(result2.status().message().empty()); + } } TEST_CASE("WifiAccessPointTimedDisable API", "[basic][rpc][client][remote][timed]") @@ -1482,4 +1516,39 @@ TEST_CASE("WifiAccessPointTimedDisable API", "[basic][rpc][client][remote][timed REQUIRE(result2.status().code() == WifiAccessPointOperationStatusCode::WifiAccessPointOperationStatusCodeOperationNotSupported); REQUIRE(result2.status().message() == "A timed disable operation is already in progress"); } + + SECTION("Succeeds when a timed disable operation is finished") + { + // Start first timed disable operation with short duration + WifiAccessPointTimedDisableRequest request1{}; + request1.set_accesspointid(InterfaceName1); + request1.set_durationseconds(1); // 1 second duration + + WifiAccessPointTimedDisableResult result1{}; + grpc::ClientContext clientContext1{}; + + grpc::Status status1; + REQUIRE_NOTHROW(status1 = client->WifiAccessPointTimedDisable(&clientContext1, request1, &result1)); + REQUIRE(status1.ok()); + REQUIRE(result1.has_status()); + REQUIRE(result1.status().code() == WifiAccessPointOperationStatusCode::WifiAccessPointOperationStatusCodeSucceeded); + + // Wait for the first operation to complete + std::this_thread::sleep_for(std::chrono::seconds(2)); + + // Start second timed disable operation after the first has finished + WifiAccessPointTimedDisableRequest request2{}; + request2.set_accesspointid(InterfaceName2); + request2.set_durationseconds(1); // 1 second duration + + WifiAccessPointTimedDisableResult result2{}; + grpc::ClientContext clientContext2{}; + + grpc::Status status2; + REQUIRE_NOTHROW(status2 = client->WifiAccessPointTimedDisable(&clientContext2, request2, &result2)); + REQUIRE(status2.ok()); + REQUIRE(result2.has_status()); + REQUIRE(result2.status().code() == WifiAccessPointOperationStatusCode::WifiAccessPointOperationStatusCodeSucceeded); + REQUIRE(result2.status().message().empty()); + } }