diff --git a/cpp/core/internal/BUILD b/cpp/core/internal/BUILD index ff017980..5667fb55 100644 --- a/cpp/core/internal/BUILD +++ b/cpp/core/internal/BUILD @@ -76,6 +76,7 @@ cc_library( "pcp_manager.h", "service_controller.h", "service_controller_router.h", + "stoppable_service_controller.h", "webrtc_bwu_handler.h", "webrtc_endpoint_channel.h", "wifi_lan_bwu_handler.h", diff --git a/cpp/core/internal/service_controller_router.cc b/cpp/core/internal/service_controller_router.cc index 61a96f2c..5e0df102 100644 --- a/cpp/core/internal/service_controller_router.cc +++ b/cpp/core/internal/service_controller_router.cc @@ -24,7 +24,9 @@ #include "core/options.h" #include "core/params.h" #include "core/payload.h" +#include "platform/base/feature_flags.h" #include "platform/public/logging.h" +#include "absl/memory/memory.h" #include "absl/time/clock.h" namespace location { @@ -48,7 +50,15 @@ const std::size_t kMaxEndpointInfoLength = 131u; ServiceControllerRouter::~ServiceControllerRouter() { NEARBY_LOG(INFO, "ServiceControllerRouter going down."); - service_controller_.reset(); + if (FeatureFlags::GetInstance() + .GetFlags() + .disable_released_service_controller) { + if (service_controller_) { + service_controller_->Shutdown(); + } + } else { + service_controller_.reset(); + } // And make sure that cleanup is the last thing we do. serializer_.Shutdown(); } @@ -427,7 +437,13 @@ void ServiceControllerRouter::ReleaseServiceControllerForClient( ClientProxy* client) { clients_.erase(client); - // service_controller_ won't be released here. Instead, in desctructor. + // service_controller_ won't be released here. Instead, in destructor. + if (FeatureFlags::GetInstance() + .GetFlags() + .disable_released_service_controller) { + service_controller_->Shutdown(); + } + if (clients_.empty()) { current_strategy_ = Strategy{}; } @@ -475,7 +491,8 @@ Status ServiceControllerRouter::UpdateCurrentServiceControllerAndStrategy( return {Status::kError}; } - service_controller_.reset(service_controller_factory_()); + service_controller_ = absl::make_unique( + service_controller_factory_()); current_strategy_ = strategy; return {Status::kSuccess}; diff --git a/cpp/core/internal/service_controller_router.h b/cpp/core/internal/service_controller_router.h index 8c9e7039..83956cd5 100644 --- a/cpp/core/internal/service_controller_router.h +++ b/cpp/core/internal/service_controller_router.h @@ -21,6 +21,7 @@ #include "core/internal/client_proxy.h" #include "core/internal/service_controller.h" +#include "core/internal/stoppable_service_controller.h" #include "core/options.h" #include "core/params.h" #include "platform/base/runnable.h" @@ -119,7 +120,7 @@ class ServiceControllerRouter { absl::flat_hash_set clients_; std::function service_controller_factory_; - std::unique_ptr service_controller_; + std::unique_ptr service_controller_; Strategy current_strategy_; SingleThreadExecutor serializer_; }; diff --git a/cpp/core/internal/service_controller_router_test.cc b/cpp/core/internal/service_controller_router_test.cc index 5af7331b..beb2c2e3 100644 --- a/cpp/core/internal/service_controller_router_test.cc +++ b/cpp/core/internal/service_controller_router_test.cc @@ -50,15 +50,10 @@ const char kFakeInejctedEndpointId[] = "abcd"; // friend class to work. class ServiceControllerRouterTest : public testing::Test { public: - ServiceControllerRouterTest() = default; - ~ServiceControllerRouterTest() override { - router_.service_controller_.release(); - } - void StartAdvertising(ClientProxy* client, std::string service_id, ConnectionOptions options, ConnectionRequestInfo info, ResultCallback callback) { - EXPECT_CALL(mock_, StartAdvertising) + EXPECT_CALL(*mock_, StartAdvertising) .WillOnce(Return(Status{Status::kSuccess})); { MutexLock lock(&mutex_); @@ -73,7 +68,7 @@ class ServiceControllerRouterTest : public testing::Test { } void StopAdvertising(ClientProxy* client, ResultCallback callback) { - EXPECT_CALL(mock_, StopAdvertising).Times(1); + EXPECT_CALL(*mock_, StopAdvertising).Times(1); { MutexLock lock(&mutex_); complete_ = false; @@ -88,7 +83,7 @@ class ServiceControllerRouterTest : public testing::Test { ConnectionOptions options, const DiscoveryListener& listener, const ResultCallback& callback) { - EXPECT_CALL(mock_, StartDiscovery) + EXPECT_CALL(*mock_, StartDiscovery) .WillOnce(Return(Status{Status::kSuccess})); { MutexLock lock(&mutex_); @@ -103,7 +98,7 @@ class ServiceControllerRouterTest : public testing::Test { } void StopDiscovery(ClientProxy* client, ResultCallback callback) { - EXPECT_CALL(mock_, StopDiscovery).Times(1); + EXPECT_CALL(*mock_, StopDiscovery).Times(1); { MutexLock lock(&mutex_); complete_ = false; @@ -117,7 +112,7 @@ class ServiceControllerRouterTest : public testing::Test { void InjectEndpoint(ClientProxy* client, std::string service_id, const OutOfBandConnectionMetadata& metadata, ResultCallback callback) { - EXPECT_CALL(mock_, InjectEndpoint).Times(1); + EXPECT_CALL(*mock_, InjectEndpoint).Times(1); { MutexLock lock(&mutex_); complete_ = false; @@ -129,7 +124,7 @@ class ServiceControllerRouterTest : public testing::Test { void RequestConnection(ClientProxy* client, const std::string& endpoint_id, const ConnectionRequestInfo& request_info, ResultCallback callback) { - EXPECT_CALL(mock_, RequestConnection) + EXPECT_CALL(*mock_, RequestConnection) .WillOnce(Return(Status{Status::kSuccess})); ConnectionOptions options; { @@ -154,7 +149,7 @@ class ServiceControllerRouterTest : public testing::Test { void AcceptConnection(ClientProxy* client, const std::string endpoint_id, const PayloadListener& listener, const ResultCallback& callback) { - EXPECT_CALL(mock_, AcceptConnection) + EXPECT_CALL(*mock_, AcceptConnection) .WillOnce(Return(Status{Status::kSuccess})); // Pre-condition for successful Accept is: connection must exist. EXPECT_TRUE(client->HasPendingConnectionToEndpoint(endpoint_id)); @@ -174,7 +169,7 @@ class ServiceControllerRouterTest : public testing::Test { void RejectConnection(ClientProxy* client, const std::string endpoint_id, ResultCallback callback) { - EXPECT_CALL(mock_, RejectConnection) + EXPECT_CALL(*mock_, RejectConnection) .WillOnce(Return(Status{Status::kSuccess})); // Pre-condition for successful Accept is: connection must exist. EXPECT_TRUE(client->HasPendingConnectionToEndpoint(endpoint_id)); @@ -192,7 +187,7 @@ class ServiceControllerRouterTest : public testing::Test { void InitiateBandwidthUpgrade(ClientProxy* client, const std::string endpoint_id, ResultCallback callback) { - EXPECT_CALL(mock_, InitiateBandwidthUpgrade).Times(1); + EXPECT_CALL(*mock_, InitiateBandwidthUpgrade).Times(1); EXPECT_TRUE(client->IsConnectedToEndpoint(endpoint_id)); { MutexLock lock(&mutex_); @@ -206,7 +201,7 @@ class ServiceControllerRouterTest : public testing::Test { void SendPayload(ClientProxy* client, const std::vector& endpoint_ids, Payload payload, ResultCallback callback) { - EXPECT_CALL(mock_, SendPayload).Times(1); + EXPECT_CALL(*mock_, SendPayload).Times(1); bool connected = false; for (const auto& endpoint_id : endpoint_ids) { @@ -225,7 +220,7 @@ class ServiceControllerRouterTest : public testing::Test { void CancelPayload(ClientProxy* client, std::int64_t payload_id, ResultCallback callback) { - EXPECT_CALL(mock_, CancelPayload) + EXPECT_CALL(*mock_, CancelPayload) .WillOnce(Return(Status{Status::kSuccess})); { MutexLock lock(&mutex_); @@ -239,7 +234,7 @@ class ServiceControllerRouterTest : public testing::Test { void DisconnectFromEndpoint(ClientProxy* client, const std::string endpoint_id, ResultCallback callback) { - EXPECT_CALL(mock_, DisconnectFromEndpoint).Times(1); + EXPECT_CALL(*mock_, DisconnectFromEndpoint).Times(1); EXPECT_TRUE(client->IsConnectedToEndpoint(endpoint_id)); { MutexLock lock(&mutex_); @@ -291,15 +286,20 @@ class ServiceControllerRouterTest : public testing::Test { ConditionVariable cond_{&mutex_}; Status result_ ABSL_GUARDED_BY(mutex_) = {Status::kError}; bool complete_ ABSL_GUARDED_BY(mutex_) = false; - MockServiceController mock_; + // `router_` will take over ownership and delete the mock + MockServiceController* mock_ = new MockServiceController(); ClientProxy client_; ServiceControllerRouter router_{ - [this]() -> ServiceController* { return &mock_; }}; + [this]() -> ServiceController* { return mock_; }}; }; namespace { -TEST_F(ServiceControllerRouterTest, CostructorDestructorWorks) { SUCCEED(); } +TEST_F(ServiceControllerRouterTest, CostructorDestructorWorks) { + // This test doesn't create `router_`, so we must clean up manually + delete mock_; + SUCCEED(); +} TEST_F(ServiceControllerRouterTest, StartAdvertisingCalled) { StartAdvertising(&client_, kServiceId, kConnectionOptions, diff --git a/cpp/core/internal/stoppable_service_controller.h b/cpp/core/internal/stoppable_service_controller.h new file mode 100644 index 00000000..4e1f1be6 --- /dev/null +++ b/cpp/core/internal/stoppable_service_controller.h @@ -0,0 +1,131 @@ +#ifndef CORE_INTERNAL_STOPPABLE_SERVICE_CONTROLLER_H_ +#define CORE_INTERNAL_STOPPABLE_SERVICE_CONTROLLER_H_ + +#include + +#include "core/internal/service_controller.h" +#include "core/status.h" +#include "platform/public/atomic_boolean.h" + +namespace location { +namespace nearby { +namespace connections { + +// A ServiceController proxy that can be shut down. +// When shut down, the API calls are not forwarded to the real controller. +// StoppableServiceController takes over ownership of ServiceController. +class StoppableServiceController : public ServiceController { + public: + explicit StoppableServiceController(ServiceController* controller) + : service_controller_{controller} {} + ~StoppableServiceController() override = default; + + void Shutdown() { stopped_.Set(true); } + + Status StartAdvertising(ClientProxy* client, const std::string& service_id, + const ConnectionOptions& options, + const ConnectionRequestInfo& info) override { + if (stopped_) { + return {Status::kError}; + } + return service_controller_->StartAdvertising(client, service_id, options, + info); + } + + void StopAdvertising(ClientProxy* client) override { + if (stopped_) { + return; + } + service_controller_->StopAdvertising(client); + } + + Status StartDiscovery(ClientProxy* client, const std::string& service_id, + const ConnectionOptions& options, + const DiscoveryListener& listener) override { + if (stopped_) { + return {Status::kError}; + } + return service_controller_->StartDiscovery(client, service_id, options, + listener); + } + void StopDiscovery(ClientProxy* client) override { + if (stopped_) { + return; + } + service_controller_->StopDiscovery(client); + } + + void InjectEndpoint(ClientProxy* client, const std::string& service_id, + const OutOfBandConnectionMetadata& metadata) override { + if (stopped_) { + return; + } + service_controller_->InjectEndpoint(client, service_id, metadata); + } + + Status RequestConnection(ClientProxy* client, const std::string& endpoint_id, + const ConnectionRequestInfo& info, + const ConnectionOptions& options) override { + if (stopped_) { + return {Status::kError}; + } + return service_controller_->RequestConnection(client, endpoint_id, info, + options); + } + Status AcceptConnection(ClientProxy* client, const std::string& endpoint_id, + const PayloadListener& listener) override { + if (stopped_) { + return {Status::kError}; + } + return service_controller_->AcceptConnection(client, endpoint_id, listener); + } + Status RejectConnection(ClientProxy* client, + const std::string& endpoint_id) override { + if (stopped_) { + return {Status::kError}; + } + return service_controller_->RejectConnection(client, endpoint_id); + } + + void InitiateBandwidthUpgrade(ClientProxy* client, + const std::string& endpoint_id) override { + if (stopped_) { + return; + } + service_controller_->InitiateBandwidthUpgrade(client, endpoint_id); + } + + void SendPayload(ClientProxy* client, + const std::vector& endpoint_ids, + Payload payload) override { + if (stopped_) { + return; + } + service_controller_->SendPayload(client, endpoint_ids, std::move(payload)); + } + + Status CancelPayload(ClientProxy* client, Payload::Id payload_id) override { + if (stopped_) { + return {Status::kError}; + } + return service_controller_->CancelPayload(client, payload_id); + } + + void DisconnectFromEndpoint(ClientProxy* client, + const std::string& endpoint_id) override { + if (stopped_) { + return; + } + service_controller_->DisconnectFromEndpoint(client, endpoint_id); + } + + private: + std::unique_ptr service_controller_; + AtomicBoolean stopped_{false}; +}; + +} // namespace connections +} // namespace nearby +} // namespace location + +#endif // CORE_INTERNAL_STOPPABLE_SERVICE_CONTROLLER_H_ diff --git a/cpp/platform/base/feature_flags.h b/cpp/platform/base/feature_flags.h index c0d9632f..7972190e 100644 --- a/cpp/platform/base/feature_flags.h +++ b/cpp/platform/base/feature_flags.h @@ -14,6 +14,10 @@ class FeatureFlags { struct Flags { bool enable_cancellation_flag = false; bool resume_before_disconnect = true; + // Disable ServiceController API (using StoppableServiceController) when + // ServiceController is released to prevent calls to that API from + // other threads. + bool disable_released_service_controller = true; }; static const FeatureFlags& GetInstance() { diff --git a/cpp/platform/public/atomic_boolean.h b/cpp/platform/public/atomic_boolean.h index 8c3f05fb..1315a3ee 100644 --- a/cpp/platform/public/atomic_boolean.h +++ b/cpp/platform/public/atomic_boolean.h @@ -38,6 +38,8 @@ class AtomicBoolean final : public api::AtomicBoolean { bool Get() const override { return impl_->Get(); } bool Set(bool value) override { return impl_->Set(value); } + explicit operator bool() const { return Get(); } + private: std::unique_ptr impl_; }; diff --git a/cpp/platform/public/atomic_boolean_test.cc b/cpp/platform/public/atomic_boolean_test.cc index 97e6825d..8dcb6ed9 100644 --- a/cpp/platform/public/atomic_boolean_test.cc +++ b/cpp/platform/public/atomic_boolean_test.cc @@ -33,6 +33,17 @@ TEST(AtomicBooleanTest, GetReturnsWhatWasSet) { EXPECT_TRUE(value.Get()); } +TEST(AtomicBooleanTest, ImplicitGetTrueValue) { + AtomicBoolean value(true); + + EXPECT_TRUE(value); +} + +TEST(AtomicBooleanTest, ImplicitGetFalseValue) { + AtomicBoolean value(false); + + EXPECT_FALSE(value); +} } // namespace } // namespace nearby } // namespace location