Merge branch 'google3' to roll forward up to cl/355791747.

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

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<StoppableServiceController>(
+      service_controller_factory_());
   current_strategy_ = strategy;
 
   return {Status::kSuccess};

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<ClientProxy*> clients_;
   std::function<ServiceController*()> service_controller_factory_;
-  std::unique_ptr<ServiceController> service_controller_;
+  std::unique_ptr<StoppableServiceController> service_controller_;
   Strategy current_strategy_;
   SingleThreadExecutor serializer_;
 };

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<std::string>& 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,

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 <memory>
+
+#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<std::string>& 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<ServiceController> service_controller_;
+  AtomicBoolean stopped_{false};
+};
+
+}  // namespace connections
+}  // namespace nearby
+}  // namespace location
+
+#endif  // CORE_INTERNAL_STOPPABLE_SERVICE_CONTROLLER_H_

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

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<api::AtomicBoolean> impl_;
 };

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