[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

@@ -15,6 +15,8 @@
 #include "core/core.h"
 
 #include <cassert>
+#include <string>
+#include <utility>
 #include <vector>
 
 #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<const std::string> endpoint_ids,
@@ -122,24 +131,24 @@ void Core::SendPayload(absl::Span<const std::string> 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

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<ServiceController*()> 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

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<int>(status.value));
-                            }});
-      }(),
+      {
+        MockServiceControllerRouter mock;
+        // Never invoke the result callback so ~Core will time out.
+        EXPECT_CALL(mock, StopAllEndpoints);
+        Core core{&mock};
+      },
       "Unable to shutdown");
 }
 

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",

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<const std::string> 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_

cpp/core/internal/service_controller_router.cc

@@ -20,13 +20,12 @@
 #include <utility>
 
 #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<std::string>& remote_endpoint_ids) {
+  for (auto& endpoint_id : remote_endpoint_ids) {
+    if (client->IsConnectedToEndpoint(endpoint_id)) {
+      return true;
+    }
+  }
+  return false;
+}
 }  // namespace
 
-ServiceControllerRouter::ServiceControllerRouter(
-    std::function<ServiceController*()> 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<ServiceController> 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<OfflineServiceController>();
   }
+  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<std::string>& 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

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<ServiceController*()> 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<const std::string> 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<const std::string> 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<ServiceController> service_controller);
 
  private:
-  static bool ClientHasConnectionToAtLeastOneEndpoint(
-      ClientProxy* client, const std::vector<std::string>& 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<ClientProxy*> clients_;
-  std::function<ServiceController*()> service_controller_factory_;
   std::unique_ptr<ServiceController> service_controller_;
-  Strategy current_strategy_;
   SingleThreadExecutor serializer_;
 };
 

cpp/core/internal/service_controller_router_test.cc

@@ -18,6 +18,7 @@
 #include <cinttypes>
 #include <memory>
 #include <string>
+#include <utility>
 
 #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<MockServiceController>();
+    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);