From f483110a8d92cf4d26f2d725a09655a0ce3b0f6c Mon Sep 17 00:00:00 2001 From: nohle Date: Fri, 6 Aug 2021 09:06:55 -0700 Subject: [PATCH] [NearbyConnections][C++] Have Cores share single ServiceControllerRouter We refactor the `Core`/`ServiceControllerRouter`/`ServiceController` classes to better align with the GmsCore implementation. Notably, 1. `Core` instances share the same `ServiceControllerRouter`. 2. `ServiceControllerRouter` directly owns one `OfflineServiceController`--that is created lazily and destroyed when when the router is destroyed--instead of using a factory method to create `ServiceController`s. 3. Removes obsolete "strategy" logic from `ServiceControllerRouter`, which used to recreate service controllers if the "strategy" changed. 4. Removes `ServiceControllerRouter::ClientDisconnecting()`, which is identical to `StopAllEndpoints()`. 5. Makes various C++ style clean-ups like removing unnecessary static variables/functions. See analogous Chrome OS changes at http://crrev.com/c/3025604. PiperOrigin-RevId: 389188124 --- cpp/core/core.cc | 39 +-- cpp/core/core.h | 13 +- cpp/core/core_test.cc | 34 +-- cpp/core/internal/BUILD | 3 +- .../internal/mock_service_controller_router.h | 101 ++++++++ .../internal/service_controller_router.cc | 245 ++++-------------- cpp/core/internal/service_controller_router.h | 102 ++++---- .../service_controller_router_test.cc | 19 +- 8 files changed, 263 insertions(+), 293 deletions(-) create mode 100644 cpp/core/internal/mock_service_controller_router.h diff --git a/cpp/core/core.cc b/cpp/core/core.cc index 3e432d01..beb85969 100644 --- a/cpp/core/core.cc +++ b/cpp/core/core.cc @@ -15,6 +15,8 @@ #include "core/core.h" #include +#include +#include #include #include "absl/time/clock.h" @@ -26,12 +28,15 @@ namespace location { namespace nearby { namespace connections { +namespace { +constexpr absl::Duration kWaitForDisconnect = absl::Milliseconds(5000); +} // namespace -constexpr absl::Duration Core::kWaitForDisconnect; +Core::Core(ServiceControllerRouter* router) : router_(router) {} Core::~Core() { CountDownLatch latch(1); - router_.ClientDisconnecting( + router_->StopAllEndpoints( &client_, { .result_cb = [&latch](Status) { latch.CountDown(); }, }); @@ -40,6 +45,10 @@ Core::~Core() { } } +Core::Core(Core&&) = default; + +Core& Core::operator=(Core&&) = default; + void Core::StartAdvertising(absl::string_view service_id, ConnectionOptions options, ConnectionRequestInfo info, @@ -47,11 +56,11 @@ void Core::StartAdvertising(absl::string_view service_id, assert(!service_id.empty()); assert(options.strategy.IsValid()); - router_.StartAdvertising(&client_, service_id, options, info, callback); + router_->StartAdvertising(&client_, service_id, options, info, callback); } void Core::StopAdvertising(const ResultCallback callback) { - router_.StopAdvertising(&client_, callback); + router_->StopAdvertising(&client_, callback); } void Core::StartDiscovery(absl::string_view service_id, @@ -60,17 +69,17 @@ void Core::StartDiscovery(absl::string_view service_id, assert(!service_id.empty()); assert(options.strategy.IsValid()); - router_.StartDiscovery(&client_, service_id, options, listener, callback); + router_->StartDiscovery(&client_, service_id, options, listener, callback); } void Core::InjectEndpoint(absl::string_view service_id, OutOfBandConnectionMetadata metadata, ResultCallback callback) { - router_.InjectEndpoint(&client_, service_id, metadata, callback); + router_->InjectEndpoint(&client_, service_id, metadata, callback); } void Core::StopDiscovery(ResultCallback callback) { - router_.StopDiscovery(&client_, callback); + router_->StopDiscovery(&client_, callback); } void Core::RequestConnection(absl::string_view endpoint_id, @@ -95,26 +104,26 @@ void Core::RequestConnection(absl::string_view endpoint_id, FeatureFlags::GetInstance().GetFlags().keep_alive_timeout_millis; } - router_.RequestConnection(&client_, endpoint_id, info, options, callback); + router_->RequestConnection(&client_, endpoint_id, info, options, callback); } void Core::AcceptConnection(absl::string_view endpoint_id, PayloadListener listener, ResultCallback callback) { assert(!endpoint_id.empty()); - router_.AcceptConnection(&client_, endpoint_id, listener, callback); + router_->AcceptConnection(&client_, endpoint_id, listener, callback); } void Core::RejectConnection(absl::string_view endpoint_id, ResultCallback callback) { assert(!endpoint_id.empty()); - router_.RejectConnection(&client_, endpoint_id, callback); + router_->RejectConnection(&client_, endpoint_id, callback); } void Core::InitiateBandwidthUpgrade(absl::string_view endpoint_id, ResultCallback callback) { - router_.InitiateBandwidthUpgrade(&client_, endpoint_id, callback); + router_->InitiateBandwidthUpgrade(&client_, endpoint_id, callback); } void Core::SendPayload(absl::Span endpoint_ids, @@ -122,24 +131,24 @@ void Core::SendPayload(absl::Span endpoint_ids, assert(payload.GetType() != Payload::Type::kUnknown); assert(!endpoint_ids.empty()); - router_.SendPayload(&client_, endpoint_ids, std::move(payload), callback); + router_->SendPayload(&client_, endpoint_ids, std::move(payload), callback); } void Core::CancelPayload(std::int64_t payload_id, ResultCallback callback) { assert(payload_id != 0); - router_.CancelPayload(&client_, payload_id, callback); + router_->CancelPayload(&client_, payload_id, callback); } void Core::DisconnectFromEndpoint(absl::string_view endpoint_id, ResultCallback callback) { assert(!endpoint_id.empty()); - router_.DisconnectFromEndpoint(&client_, endpoint_id, callback); + router_->DisconnectFromEndpoint(&client_, endpoint_id, callback); } void Core::StopAllEndpoints(ResultCallback callback) { - router_.StopAllEndpoints(&client_, callback); + router_->StopAllEndpoints(&client_, callback); } } // namespace connections diff --git a/cpp/core/core.h b/cpp/core/core.h index 0a594852..ea18667c 100644 --- a/cpp/core/core.h +++ b/cpp/core/core.h @@ -20,7 +20,6 @@ #include "absl/strings/string_view.h" #include "absl/types/span.h" #include "core/internal/client_proxy.h" -#include "core/internal/offline_service_controller.h" #include "core/internal/service_controller.h" #include "core/internal/service_controller_router.h" #include "core/listeners.h" @@ -34,12 +33,10 @@ namespace connections { // This class defines the API of the Nearby Connections Core library. class Core { public: - explicit Core(std::function factory = - []() { return new OfflineServiceController; }) - : router_(factory) {} + explicit Core(ServiceControllerRouter* router); ~Core(); - Core(Core&&) = default; - Core& operator=(Core&&) = default; + Core(Core&&); + Core& operator=(Core&&); // Starts advertising an endpoint for a local app. // @@ -234,10 +231,8 @@ class Core { std::string GetLocalEndpointId() { return client_.GetLocalEndpointId(); } private: - static constexpr absl::Duration kWaitForDisconnect = absl::Milliseconds(5000); - ClientProxy client_; - ServiceControllerRouter router_; + ServiceControllerRouter* router_ = nullptr; }; } // namespace connections diff --git a/cpp/core/core_test.cc b/cpp/core/core_test.cc index ca5aa698..982baae7 100644 --- a/cpp/core/core_test.cc +++ b/cpp/core/core_test.cc @@ -17,9 +17,7 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" #include "absl/time/clock.h" -#include "core/internal/client_proxy.h" -#include "core/internal/mock_service_controller.h" -#include "core/internal/service_controller.h" +#include "core/internal/mock_service_controller_router.h" #include "platform/public/logging.h" namespace location { @@ -28,27 +26,23 @@ namespace connections { namespace { TEST(CoreTest, ConstructorDestructorWorks) { - MockServiceController mock; - Core core{[&mock]() { return &mock; }}; + MockServiceControllerRouter mock; + // Called when Core is destroyed. + EXPECT_CALL(mock, StopAllEndpoints) + .WillOnce([&](ClientProxy* client, const ResultCallback& callback) { + callback.result_cb({Status::kSuccess}); + }); + Core core{&mock}; } TEST(CoreTest, DestructorReportsFatalFailure) { - MockServiceController mock; - ON_CALL(mock, StopDiscovery).WillByDefault([](ClientProxy* client) { - NEARBY_LOG(INFO, "Blocking Endpoint disconnect for 10 sec"); - absl::SleepFor(absl::Milliseconds(10000)); - }); ASSERT_DEATH( - [&mock]() { - Core core{[&mock]() { return &mock; }}; - EXPECT_CALL(mock, StartDiscovery).Times(1); - EXPECT_CALL(mock, StopAdvertising).Times(1); - core.StartDiscovery("service_id", {.strategy = Strategy::kP2pCluster}, - {}, {.result_cb = [](Status status) { - NEARBY_LOG(INFO, "Discovery status: %d", - static_cast(status.value)); - }}); - }(), + { + MockServiceControllerRouter mock; + // Never invoke the result callback so ~Core will time out. + EXPECT_CALL(mock, StopAllEndpoints); + Core core{&mock}; + }, "Unable to shutdown"); } diff --git a/cpp/core/internal/BUILD b/cpp/core/internal/BUILD index 94912369..d039f3b1 100644 --- a/cpp/core/internal/BUILD +++ b/cpp/core/internal/BUILD @@ -139,6 +139,7 @@ cc_library( ], hdrs = [ "mock_service_controller.h", + "mock_service_controller_router.h", "offline_simulation_user.h", "simulation_user.h", ], @@ -147,7 +148,7 @@ cc_library( ], deps = [ ":internal", - "//testing/base/public:gunit", + "//testing/base/public:gunit_for_library_testonly", "//absl/functional:bind_front", "//absl/strings", "//core:core_types", diff --git a/cpp/core/internal/mock_service_controller_router.h b/cpp/core/internal/mock_service_controller_router.h new file mode 100644 index 00000000..32d8d445 --- /dev/null +++ b/cpp/core/internal/mock_service_controller_router.h @@ -0,0 +1,101 @@ +// Copyright 2020 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// https://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef CORE_INTERNAL_MOCK_SERVICE_CONTROLLER_ROUTER_H_ +#define CORE_INTERNAL_MOCK_SERVICE_CONTROLLER_ROUTER_H_ + +#include "gmock/gmock.h" +#include "core/internal/service_controller_router.h" + +namespace location { +namespace nearby { +namespace connections { + +class MockServiceControllerRouter : public ServiceControllerRouter { + public: + MOCK_METHOD(void, StartAdvertising, + (ClientProxy * client, absl::string_view service_id, + const ConnectionOptions& options, + const ConnectionRequestInfo& info, + const ResultCallback& callback), + (override)); + + MOCK_METHOD(void, StopAdvertising, + (ClientProxy * client, const ResultCallback& callback), + (override)); + + MOCK_METHOD(void, StartDiscovery, + (ClientProxy * client, absl::string_view service_id, + const ConnectionOptions& options, + const DiscoveryListener& listener, + const ResultCallback& callback), + (override)); + + MOCK_METHOD(void, StopDiscovery, + (ClientProxy * client, const ResultCallback& callback), + (override)); + + MOCK_METHOD(void, InjectEndpoint, + (ClientProxy * client, absl::string_view service_id, + const OutOfBandConnectionMetadata& metadata, + const ResultCallback& callback), + (override)); + + MOCK_METHOD(void, RequestConnection, + (ClientProxy * client, absl::string_view endpoint_id, + const ConnectionRequestInfo& info, + const ConnectionOptions& options, + const ResultCallback& callback), + (override)); + + MOCK_METHOD(void, AcceptConnection, + (ClientProxy * client, absl::string_view endpoint_id, + const PayloadListener& listener, const ResultCallback& callback), + (override)); + + MOCK_METHOD(void, RejectConnection, + (ClientProxy * client, absl::string_view endpoint_id, + const ResultCallback& callback), + (override)); + + MOCK_METHOD(void, InitiateBandwidthUpgrade, + (ClientProxy * client, absl::string_view endpoint_id, + const ResultCallback& callback), + (override)); + + MOCK_METHOD(void, SendPayload, + (ClientProxy * client, absl::Span endpoint_ids, + Payload payload, const ResultCallback& callback), + (override)); + + MOCK_METHOD(void, CancelPayload, + (ClientProxy * client, std::uint64_t payload_id, + const ResultCallback& callback), + (override)); + + MOCK_METHOD(void, DisconnectFromEndpoint, + (ClientProxy * client, absl::string_view endpoint_id, + const ResultCallback& callback), + (override)); + + MOCK_METHOD(void, StopAllEndpoints, + (ClientProxy * client, const ResultCallback& callback), + (override)); +}; + +} // namespace connections +} // namespace nearby +} // namespace location + +#endif // CORE_INTERNAL_MOCK_SERVICE_CONTROLLER_ROUTER_H_ diff --git a/cpp/core/internal/service_controller_router.cc b/cpp/core/internal/service_controller_router.cc index 124ded85..7c5eaffd 100644 --- a/cpp/core/internal/service_controller_router.cc +++ b/cpp/core/internal/service_controller_router.cc @@ -20,13 +20,12 @@ #include #include "absl/memory/memory.h" -#include "absl/time/clock.h" #include "core/internal/client_proxy.h" +#include "core/internal/offline_service_controller.h" #include "core/listeners.h" #include "core/options.h" #include "core/params.h" #include "core/payload.h" -#include "platform/base/feature_flags.h" #include "platform/public/logging.h" namespace location { @@ -45,11 +44,19 @@ const std::size_t kEndpointIdLength = 4u; // advertised by one device and can be used by the other device to identify the // advertiser. const std::size_t kMaxEndpointInfoLength = 131u; + +bool ClientHasConnectionToAtLeastOneEndpoint( + ClientProxy* client, const std::vector& remote_endpoint_ids) { + for (auto& endpoint_id : remote_endpoint_ids) { + if (client->IsConnectedToEndpoint(endpoint_id)) { + return true; + } + } + return false; +} } // namespace -ServiceControllerRouter::ServiceControllerRouter( - std::function factory) - : service_controller_factory_(std::move(factory)) { +ServiceControllerRouter::ServiceControllerRouter() { NEARBY_LOGS(INFO) << "ServiceControllerRouter going up."; } @@ -71,29 +78,21 @@ void ServiceControllerRouter::StartAdvertising( "scr-start-advertising", [this, client, service_id = std::string(service_id), options, info, callback]() { - Status status = - AcquireServiceControllerForClient(client, options.strategy); - if (!status.Ok()) { - callback.result_cb(status); - return; - } - if (client->IsAdvertising()) { callback.result_cb({Status::kAlreadyAdvertising}); return; } - status = service_controller_->StartAdvertising(client, service_id, - options, info); - callback.result_cb(status); + callback.result_cb(GetServiceController()->StartAdvertising( + client, service_id, options, info)); }); } void ServiceControllerRouter::StopAdvertising(ClientProxy* client, const ResultCallback& callback) { RouteToServiceController("scr-stop-advertising", [this, client, callback]() { - if (ClientHasAcquiredServiceController(client) && client->IsAdvertising()) { - service_controller_->StopAdvertising(client); + if (client->IsAdvertising()) { + GetServiceController()->StopAdvertising(client); } callback.result_cb({Status::kSuccess}); }); @@ -108,29 +107,21 @@ void ServiceControllerRouter::StartDiscovery(ClientProxy* client, "scr-start-discovery", [this, client, service_id = std::string(service_id), options, listener, callback]() { - Status status = - AcquireServiceControllerForClient(client, options.strategy); - if (!status.Ok()) { - callback.result_cb(status); - return; - } - if (client->IsDiscovering()) { callback.result_cb({Status::kAlreadyDiscovering}); return; } - status = service_controller_->StartDiscovery(client, service_id, - options, listener); - callback.result_cb(status); + callback.result_cb(GetServiceController()->StartDiscovery( + client, service_id, options, listener)); }); } void ServiceControllerRouter::StopDiscovery(ClientProxy* client, const ResultCallback& callback) { RouteToServiceController("scr-stop-discovery", [this, client, callback]() { - if (ClientHasAcquiredServiceController(client) && client->IsDiscovering()) { - service_controller_->StopDiscovery(client); + if (client->IsDiscovering()) { + GetServiceController()->StopDiscovery(client); } callback.result_cb({Status::kSuccess}); }); @@ -163,13 +154,12 @@ void ServiceControllerRouter::InjectEndpoint( return; } - if (!ClientHasAcquiredServiceController(client) || - !client->IsDiscovering()) { + if (!client->IsDiscovering()) { callback.result_cb({Status::kOutOfOrderApiCall}); return; } - service_controller_->InjectEndpoint(client, service_id, metadata); + GetServiceController()->InjectEndpoint(client, service_id, metadata); callback.result_cb({Status::kSuccess}); }); } @@ -186,18 +176,13 @@ void ServiceControllerRouter::RequestConnection( "scr-request-connection", [this, client, endpoint_id = std::string(endpoint_id), info, options, callback]() { - if (!ClientHasAcquiredServiceController(client)) { - callback.result_cb({Status::kOutOfOrderApiCall}); - return; - } - if (client->HasPendingConnectionToEndpoint(endpoint_id) || client->IsConnectedToEndpoint(endpoint_id)) { callback.result_cb({Status::kAlreadyConnectedToEndpoint}); return; } - Status status = service_controller_->RequestConnection( + Status status = GetServiceController()->RequestConnection( client, endpoint_id, info, options); if (!status.Ok()) { client->CancelEndpoint(endpoint_id); @@ -214,11 +199,6 @@ void ServiceControllerRouter::AcceptConnection(ClientProxy* client, "scr-accept-connection", [this, client, endpoint_id = std::string(endpoint_id), listener, callback]() { - if (!ClientHasAcquiredServiceController(client)) { - callback.result_cb({Status::kOutOfOrderApiCall}); - return; - } - if (client->IsConnectedToEndpoint(endpoint_id)) { callback.result_cb({Status::kAlreadyConnectedToEndpoint}); return; @@ -234,7 +214,7 @@ void ServiceControllerRouter::AcceptConnection(ClientProxy* client, return; } - callback.result_cb(service_controller_->AcceptConnection( + callback.result_cb(GetServiceController()->AcceptConnection( client, endpoint_id, listener)); }); } @@ -247,11 +227,6 @@ void ServiceControllerRouter::RejectConnection(ClientProxy* client, RouteToServiceController( "scr-reject-connection", [this, client, endpoint_id = std::string(endpoint_id), callback]() { - if (!ClientHasAcquiredServiceController(client)) { - callback.result_cb({Status::kOutOfOrderApiCall}); - return; - } - if (client->IsConnectedToEndpoint(endpoint_id)) { callback.result_cb({Status::kAlreadyConnectedToEndpoint}); return; @@ -268,7 +243,7 @@ void ServiceControllerRouter::RejectConnection(ClientProxy* client, } callback.result_cb( - service_controller_->RejectConnection(client, endpoint_id)); + GetServiceController()->RejectConnection(client, endpoint_id)); }); } @@ -278,13 +253,12 @@ void ServiceControllerRouter::InitiateBandwidthUpgrade( RouteToServiceController( "scr-init-bwu", [this, client, endpoint_id = std::string(endpoint_id), callback]() { - if (!ClientHasAcquiredServiceController(client) || - !client->IsConnectedToEndpoint(endpoint_id)) { + if (!client->IsConnectedToEndpoint(endpoint_id)) { callback.result_cb({Status::kOutOfOrderApiCall}); return; } - service_controller_->InitiateBandwidthUpgrade(client, endpoint_id); + GetServiceController()->InitiateBandwidthUpgrade(client, endpoint_id); // Operation is triggered; the caller can listen to // ConnectionListener::OnBandwidthChanged() to determine its success. @@ -307,18 +281,13 @@ void ServiceControllerRouter::SendPayload( RouteToServiceController("scr-send-payload", [this, client, shared_payload, endpoints, callback]() { - if (!ClientHasAcquiredServiceController(client)) { - callback.result_cb({Status::kOutOfOrderApiCall}); - return; - } - if (!ClientHasConnectionToAtLeastOneEndpoint(client, endpoints)) { callback.result_cb({Status::kEndpointUnknown}); return; } - service_controller_->SendPayload(client, endpoints, - std::move(*shared_payload)); + GetServiceController()->SendPayload(client, endpoints, + std::move(*shared_payload)); // At this point, we've queued up the send Payload request with the // ServiceController; any further failures (e.g. one of the endpoints is @@ -331,15 +300,11 @@ void ServiceControllerRouter::SendPayload( void ServiceControllerRouter::CancelPayload(ClientProxy* client, std::uint64_t payload_id, const ResultCallback& callback) { - RouteToServiceController("scr-cancel-payload", [this, client, payload_id, - callback]() { - if (!ClientHasAcquiredServiceController(client)) { - callback.result_cb({Status::kOutOfOrderApiCall}); - return; - } - - callback.result_cb(service_controller_->CancelPayload(client, payload_id)); - }); + RouteToServiceController( + "scr-cancel-payload", [this, client, payload_id, callback]() { + callback.result_cb( + GetServiceController()->CancelPayload(client, payload_id)); + }); } void ServiceControllerRouter::DisconnectFromEndpoint( @@ -352,15 +317,14 @@ void ServiceControllerRouter::DisconnectFromEndpoint( RouteToServiceController( "scr-disconnect-endpoint", [this, client, endpoint_id = std::string(endpoint_id), callback]() { - if (ClientHasAcquiredServiceController(client)) { - if (!client->IsConnectedToEndpoint(endpoint_id) && - !client->HasPendingConnectionToEndpoint(endpoint_id)) { - callback.result_cb({Status::kOutOfOrderApiCall}); - return; - } - service_controller_->DisconnectFromEndpoint(client, endpoint_id); - callback.result_cb({Status::kSuccess}); + if (!client->IsConnectedToEndpoint(endpoint_id) && + !client->HasPendingConnectionToEndpoint(endpoint_id)) { + callback.result_cb({Status::kOutOfOrderApiCall}); + return; } + + GetServiceController()->DisconnectFromEndpoint(client, endpoint_id); + callback.result_cb({Status::kSuccess}); }); } @@ -375,111 +339,39 @@ void ServiceControllerRouter::StopAllEndpoints(ClientProxy* client, NEARBY_LOGS(INFO) << "Client " << client->GetClientId() << " has requested us to stop all endpoints. We will " "now reset the client."; - if (ClientHasAcquiredServiceController(client)) { - DoneWithStrategySessionForClient(client); - } + FinishClientSession(client); callback.result_cb({Status::kSuccess}); }); } -void ServiceControllerRouter::ClientDisconnecting( - ClientProxy* client, const ResultCallback& callback) { - // Client can emit the cancellation at anytime, we need to execute the request - // without further posting it. - client->CancelAllEndpoints(); - - RouteToServiceController( - "scr-client-disconnecting", [this, client, callback]() { - if (ClientHasAcquiredServiceController(client)) { - DoneWithStrategySessionForClient(client); - NEARBY_LOGS(INFO) << "Client " << client->GetClientId() - << " has completed the client's connection."; - } - callback.result_cb({Status::kSuccess}); - }); +void ServiceControllerRouter::SetServiceControllerForTesting( + std::unique_ptr service_controller) { + service_controller_ = std::move(service_controller); } -Status ServiceControllerRouter::AcquireServiceControllerForClient( - ClientProxy* client, Strategy strategy) { - if (current_strategy_.IsNone()) { - // Case 1: There is no existing Strategy at all. - - // Set everything up for the first time. - Status status = UpdateCurrentServiceControllerAndStrategy(strategy); - if (!status.Ok()) { - return status; - } - clients_.insert(client); - return {Status::kSuccess}; - } else if (strategy == current_strategy_) { - // Case 2: The existing Strategy matches. - - // The new client just needs to be added to the set of clients using the - // current ServiceController. - clients_.insert(client); - return {Status::kSuccess}; - } else { - // Case 3: The existing Strategy doesn't match. - - // It's only safe for a client to cause a switch if it's the only client - // using the current ServiceController. - bool is_the_only_client_of_service_controller = - clients_.size() == 1 && ClientHasAcquiredServiceController(client); - if (!is_the_only_client_of_service_controller) { - NEARBY_LOGS(INFO) << "Client has already active strategy."; - return {Status::kAlreadyHaveActiveStrategy}; - } - - // If the client still has connected endpoints, they must disconnect before - // they can switch. - if (!client->GetConnectedEndpoints().empty()) { - NEARBY_LOGS(INFO) << "Client has connected endpoints."; - return {Status::kOutOfOrderApiCall}; - } - - // By this point, it's safe to switch the Strategy and ServiceController - // (and since it's the only client, there's no need to add it to the set of - // clients using the current ServiceController). - return UpdateCurrentServiceControllerAndStrategy(strategy); +ServiceController* ServiceControllerRouter::GetServiceController() { + if (!service_controller_) { + service_controller_ = std::make_unique(); } + return service_controller_.get(); } -bool ServiceControllerRouter::ClientHasAcquiredServiceController( - ClientProxy* client) const { - return clients_.contains(client); -} - -void ServiceControllerRouter::ReleaseServiceControllerForClient( - ClientProxy* client) { - clients_.erase(client); - - // service_controller_ won't be released here. Instead, in destructor. - service_controller_->Stop(); - - if (clients_.empty()) { - current_strategy_ = Strategy{}; - } -} - -/** Clean up all state for this client. The client is now free to switch - * strategies. */ -void ServiceControllerRouter::DoneWithStrategySessionForClient( - ClientProxy* client) { +void ServiceControllerRouter::FinishClientSession(ClientProxy* client) { // Disconnect from all the connected endpoints tied to this clientProxy. for (auto& endpoint_id : client->GetPendingConnectedEndpoints()) { - service_controller_->DisconnectFromEndpoint(client, endpoint_id); + GetServiceController()->DisconnectFromEndpoint(client, endpoint_id); } for (auto& endpoint_id : client->GetConnectedEndpoints()) { - service_controller_->DisconnectFromEndpoint(client, endpoint_id); + GetServiceController()->DisconnectFromEndpoint(client, endpoint_id); } - // Stop any advertising and discovery that may be underway due to this - // clientProxy. - service_controller_->StopAdvertising(client); - service_controller_->StopDiscovery(client); + // Stop any advertising and discovery that may be underway due to this client. + GetServiceController()->StopAdvertising(client); + GetServiceController()->StopDiscovery(client); - ReleaseServiceControllerForClient(client); + // Finally, clear all state maintained by this client. + client->Reset(); } void ServiceControllerRouter::RouteToServiceController(const std::string& name, @@ -487,29 +379,6 @@ void ServiceControllerRouter::RouteToServiceController(const std::string& name, serializer_.Execute(name, std::move(runnable)); } -bool ServiceControllerRouter::ClientHasConnectionToAtLeastOneEndpoint( - ClientProxy* client, const std::vector& remote_endpoint_ids) { - for (auto& endpoint_id : remote_endpoint_ids) { - if (client->IsConnectedToEndpoint(endpoint_id)) { - return true; - } - } - return false; -} - -Status ServiceControllerRouter::UpdateCurrentServiceControllerAndStrategy( - Strategy strategy) { - if (!strategy.IsValid()) { - NEARBY_LOGS(INFO) << "Strategy is not valid."; - return {Status::kError}; - } - - service_controller_.reset(service_controller_factory_()); - current_strategy_ = strategy; - - return {Status::kSuccess}; -} - } // namespace connections } // namespace nearby } // namespace location diff --git a/cpp/core/internal/service_controller_router.h b/cpp/core/internal/service_controller_router.h index cf4e508a..54520ac2 100644 --- a/cpp/core/internal/service_controller_router.h +++ b/cpp/core/internal/service_controller_router.h @@ -53,71 +53,73 @@ namespace connections { // of a ServiceController interface, which does the actual job. class ServiceControllerRouter { public: - explicit ServiceControllerRouter(std::function factory); - ~ServiceControllerRouter(); - ServiceControllerRouter(ServiceControllerRouter&&) = default; - ServiceControllerRouter& operator=(ServiceControllerRouter&&) = default; + ServiceControllerRouter(); + virtual ~ServiceControllerRouter(); + // Not copyable or movable + ServiceControllerRouter(const ServiceControllerRouter&) = delete; + ServiceControllerRouter& operator=(const ServiceControllerRouter&) = delete; + ServiceControllerRouter(ServiceControllerRouter&&) = delete; + ServiceControllerRouter& operator=(ServiceControllerRouter&&) = delete; - void StartAdvertising(ClientProxy* client, absl::string_view service_id, - const ConnectionOptions& options, - const ConnectionRequestInfo& info, - const ResultCallback& callback); - void StopAdvertising(ClientProxy* client, const ResultCallback& callback); + virtual void StartAdvertising(ClientProxy* client, + absl::string_view service_id, + const ConnectionOptions& options, + const ConnectionRequestInfo& info, + const ResultCallback& callback); + virtual void StopAdvertising(ClientProxy* client, + const ResultCallback& callback); - void StartDiscovery(ClientProxy* client, absl::string_view service_id, - const ConnectionOptions& options, - const DiscoveryListener& listener, - const ResultCallback& callback); - void StopDiscovery(ClientProxy* client, const ResultCallback& callback); + virtual void StartDiscovery(ClientProxy* client, absl::string_view service_id, + const ConnectionOptions& options, + const DiscoveryListener& listener, + const ResultCallback& callback); + virtual void StopDiscovery(ClientProxy* client, + const ResultCallback& callback); - void InjectEndpoint(ClientProxy* client, absl::string_view service_id, - const OutOfBandConnectionMetadata& metadata, - const ResultCallback& callback); + virtual void InjectEndpoint(ClientProxy* client, absl::string_view service_id, + const OutOfBandConnectionMetadata& metadata, + const ResultCallback& callback); - void RequestConnection(ClientProxy* client, absl::string_view endpoint_id, - const ConnectionRequestInfo& info, - const ConnectionOptions& options, - const ResultCallback& callback); - void AcceptConnection(ClientProxy* client, absl::string_view endpoint_id, - const PayloadListener& listener, - const ResultCallback& callback); - void RejectConnection(ClientProxy* client, absl::string_view endpoint_id, - const ResultCallback& callback); - - void InitiateBandwidthUpgrade(ClientProxy* client, + virtual void RequestConnection(ClientProxy* client, + absl::string_view endpoint_id, + const ConnectionRequestInfo& info, + const ConnectionOptions& options, + const ResultCallback& callback); + virtual void AcceptConnection(ClientProxy* client, + absl::string_view endpoint_id, + const PayloadListener& listener, + const ResultCallback& callback); + virtual void RejectConnection(ClientProxy* client, absl::string_view endpoint_id, const ResultCallback& callback); - void SendPayload(ClientProxy* client, - absl::Span endpoint_ids, Payload payload, - const ResultCallback& callback); - void CancelPayload(ClientProxy* client, std::uint64_t payload_id, - const ResultCallback& callback); + virtual void InitiateBandwidthUpgrade(ClientProxy* client, + absl::string_view endpoint_id, + const ResultCallback& callback); - void DisconnectFromEndpoint(ClientProxy* client, - absl::string_view endpoint_id, - const ResultCallback& callback); - void StopAllEndpoints(ClientProxy* client, const ResultCallback& callback); + virtual void SendPayload(ClientProxy* client, + absl::Span endpoint_ids, + Payload payload, const ResultCallback& callback); + virtual void CancelPayload(ClientProxy* client, std::uint64_t payload_id, + const ResultCallback& callback); - void ClientDisconnecting(ClientProxy* client, const ResultCallback& callback); + virtual void DisconnectFromEndpoint(ClientProxy* client, + absl::string_view endpoint_id, + const ResultCallback& callback); + virtual void StopAllEndpoints(ClientProxy* client, + const ResultCallback& callback); + + void SetServiceControllerForTesting( + std::unique_ptr service_controller); private: - static bool ClientHasConnectionToAtLeastOneEndpoint( - ClientProxy* client, const std::vector& remote_endpoint_ids); + // Lazily create ServiceController. + ServiceController* GetServiceController(); void RouteToServiceController(const std::string& name, Runnable runnable); + void FinishClientSession(ClientProxy* client); - Status AcquireServiceControllerForClient(ClientProxy* client, - Strategy strategy); - bool ClientHasAcquiredServiceController(ClientProxy* client) const; - void ReleaseServiceControllerForClient(ClientProxy* client); - void DoneWithStrategySessionForClient(ClientProxy* client); - Status UpdateCurrentServiceControllerAndStrategy(Strategy strategy); - - absl::flat_hash_set clients_; - std::function service_controller_factory_; 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 f0f4942f..9f6f8b8d 100644 --- a/cpp/core/internal/service_controller_router_test.cc +++ b/cpp/core/internal/service_controller_router_test.cc @@ -18,6 +18,7 @@ #include #include #include +#include #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -50,6 +51,12 @@ const char kFakeInejctedEndpointId[] = "abcd"; // friend class to work. class ServiceControllerRouterTest : public testing::Test { public: + void SetUp() override { + auto mock = std::make_unique(); + mock_ = mock.get(); + router_.SetServiceControllerForTesting(std::move(mock)); + } + void StartAdvertising(ClientProxy* client, std::string service_id, ConnectionOptions options, ConnectionRequestInfo info, ResultCallback callback) { @@ -286,21 +293,13 @@ class ServiceControllerRouterTest : public testing::Test { ConditionVariable cond_{&mutex_}; Status result_ ABSL_GUARDED_BY(mutex_) = {Status::kError}; bool complete_ ABSL_GUARDED_BY(mutex_) = false; - // `router_` will take over ownership and delete the mock - MockServiceController* mock_ = new MockServiceController(); + MockServiceController* mock_; ClientProxy client_; - ServiceControllerRouter router_{ - [this]() -> ServiceController* { return mock_; }}; + ServiceControllerRouter router_; }; namespace { -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, kConnectionRequestInfo, kCallback);