From 5bc388a00a56733a991abaab375867c051cefb6c Mon Sep 17 00:00:00 2001 From: hai007 Date: Mon, 8 Feb 2021 03:07:16 -0800 Subject: [PATCH 1/6] Roll forward up to cl/355557269. --- cpp/core/internal/base_pcp_handler.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/core/internal/base_pcp_handler.cc b/cpp/core/internal/base_pcp_handler.cc index 258012c7..cb4382d8 100644 --- a/cpp/core/internal/base_pcp_handler.cc +++ b/cpp/core/internal/base_pcp_handler.cc @@ -185,7 +185,7 @@ Status BasePcpHandler::WaitForResult(const std::string& method_name, NEARBY_LOG(INFO, "No future to wait for; return with error"); return {Status::kError}; } - NEARBY_LOG(INFO, "waiting for future to complete"); + NEARBY_LOG(INFO, "Waiting for future to complete: %s", method_name.c_str()); ExceptionOr result = future->Get(); if (!result.ok()) { NEARBY_LOG(INFO, "Future:[%s] completed with exception: %d", From 84efda086827147a8c58986ed94bf008f404b32e Mon Sep 17 00:00:00 2001 From: hai007 Date: Mon, 8 Feb 2021 03:56:54 -0800 Subject: [PATCH 2/6] Roll forward up to cl/355791747. --- cpp/core/internal/BUILD | 1 + .../internal/service_controller_router.cc | 23 ++- cpp/core/internal/service_controller_router.h | 3 +- .../service_controller_router_test.cc | 40 +++--- .../internal/stoppable_service_controller.h | 131 ++++++++++++++++++ cpp/platform/base/feature_flags.h | 4 + cpp/platform/public/atomic_boolean.h | 2 + cpp/platform/public/atomic_boolean_test.cc | 11 ++ 8 files changed, 191 insertions(+), 24 deletions(-) create mode 100644 cpp/core/internal/stoppable_service_controller.h diff --git a/cpp/core/internal/BUILD b/cpp/core/internal/BUILD index f6829072..803df6ad 100644 --- a/cpp/core/internal/BUILD +++ b/cpp/core/internal/BUILD @@ -62,6 +62,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 da189345..e28d842d 100644 --- a/cpp/core/internal/service_controller_router.cc +++ b/cpp/core/internal/service_controller_router.cc @@ -10,7 +10,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 { @@ -34,7 +36,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(); } @@ -413,7 +423,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{}; } @@ -461,7 +477,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 26eb7bf2..9a2817e2 100644 --- a/cpp/core/internal/service_controller_router.h +++ b/cpp/core/internal/service_controller_router.h @@ -7,6 +7,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" @@ -105,7 +106,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 36967b98..2dcee195 100644 --- a/cpp/core/internal/service_controller_router_test.cc +++ b/cpp/core/internal/service_controller_router_test.cc @@ -36,15 +36,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_); @@ -59,7 +54,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; @@ -74,7 +69,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_); @@ -89,7 +84,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; @@ -103,7 +98,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; @@ -115,7 +110,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; { @@ -140,7 +135,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)); @@ -160,7 +155,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)); @@ -178,7 +173,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_); @@ -192,7 +187,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) { @@ -211,7 +206,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_); @@ -225,7 +220,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_); @@ -277,15 +272,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 992bbe0f..c77d7a8a 100644 --- a/cpp/platform/public/atomic_boolean.h +++ b/cpp/platform/public/atomic_boolean.h @@ -24,6 +24,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 a25e7463..9e728a9c 100644 --- a/cpp/platform/public/atomic_boolean_test.cc +++ b/cpp/platform/public/atomic_boolean_test.cc @@ -19,6 +19,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 From 4b015aefd81ad7e95da6c80a7c1e4309c1e8049f Mon Sep 17 00:00:00 2001 From: hai007 Date: Mon, 8 Feb 2021 04:15:13 -0800 Subject: [PATCH 3/6] OSS fixes. --- cpp/core/internal/fuzzers/BUILD | 14 ++++++++++++++ cpp/core/internal/fuzzers/offline_frames_fuzzer.cc | 14 ++++++++++++++ cpp/core/internal/stoppable_service_controller.h | 14 ++++++++++++++ cpp/platform/base/cancellation_flag.cc | 14 ++++++++++++++ cpp/platform/base/cancellation_flag.h | 14 ++++++++++++++ cpp/platform/base/cancellation_flag_test.cc | 14 ++++++++++++++ cpp/platform/base/feature_flags.h | 14 ++++++++++++++ cpp/platform/base/feature_flags_test.cc | 14 ++++++++++++++ 8 files changed, 112 insertions(+) diff --git a/cpp/core/internal/fuzzers/BUILD b/cpp/core/internal/fuzzers/BUILD index 0c113e12..ca304322 100644 --- a/cpp/core/internal/fuzzers/BUILD +++ b/cpp/core/internal/fuzzers/BUILD @@ -1,3 +1,17 @@ +# 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. + load("//security/fuzzing/blaze:cc_fuzz_target.bzl", "cc_fuzz_target") cc_fuzz_target( diff --git a/cpp/core/internal/fuzzers/offline_frames_fuzzer.cc b/cpp/core/internal/fuzzers/offline_frames_fuzzer.cc index 50a6062d..432970d2 100644 --- a/cpp/core/internal/fuzzers/offline_frames_fuzzer.cc +++ b/cpp/core/internal/fuzzers/offline_frames_fuzzer.cc @@ -1,3 +1,17 @@ +// 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. + #include "core/internal/offline_frames.h" #include "platform/base/byte_array.h" diff --git a/cpp/core/internal/stoppable_service_controller.h b/cpp/core/internal/stoppable_service_controller.h index 4e1f1be6..3a46e165 100644 --- a/cpp/core/internal/stoppable_service_controller.h +++ b/cpp/core/internal/stoppable_service_controller.h @@ -1,3 +1,17 @@ +// 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_STOPPABLE_SERVICE_CONTROLLER_H_ #define CORE_INTERNAL_STOPPABLE_SERVICE_CONTROLLER_H_ diff --git a/cpp/platform/base/cancellation_flag.cc b/cpp/platform/base/cancellation_flag.cc index f317355b..7aaf4d2d 100644 --- a/cpp/platform/base/cancellation_flag.cc +++ b/cpp/platform/base/cancellation_flag.cc @@ -1,3 +1,17 @@ +// 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. + #include "platform/base/cancellation_flag.h" #include "platform/base/feature_flags.h" diff --git a/cpp/platform/base/cancellation_flag.h b/cpp/platform/base/cancellation_flag.h index 4ef3f1ba..a9cd9450 100644 --- a/cpp/platform/base/cancellation_flag.h +++ b/cpp/platform/base/cancellation_flag.h @@ -1,3 +1,17 @@ +// 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 PLATFORM_BASE_CANCELLATION_FLAG_H_ #define PLATFORM_BASE_CANCELLATION_FLAG_H_ diff --git a/cpp/platform/base/cancellation_flag_test.cc b/cpp/platform/base/cancellation_flag_test.cc index 80bb64a6..e5f05af9 100644 --- a/cpp/platform/base/cancellation_flag_test.cc +++ b/cpp/platform/base/cancellation_flag_test.cc @@ -1,3 +1,17 @@ +// 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. + #include "platform/base/cancellation_flag.h" #include "platform/base/feature_flags.h" diff --git a/cpp/platform/base/feature_flags.h b/cpp/platform/base/feature_flags.h index 7972190e..ceb0c173 100644 --- a/cpp/platform/base/feature_flags.h +++ b/cpp/platform/base/feature_flags.h @@ -1,3 +1,17 @@ +// 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 PLATFORM_BASE_FEATURE_FLAGS_H_ #define PLATFORM_BASE_FEATURE_FLAGS_H_ diff --git a/cpp/platform/base/feature_flags_test.cc b/cpp/platform/base/feature_flags_test.cc index d585a7fc..2ad5e44f 100644 --- a/cpp/platform/base/feature_flags_test.cc +++ b/cpp/platform/base/feature_flags_test.cc @@ -1,3 +1,17 @@ +// 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. + #include "platform/base/feature_flags.h" #include "platform/base/medium_environment.h" From 746794e23cac098e2fa21fac9c9976d3b5dad5a9 Mon Sep 17 00:00:00 2001 From: hai007 Date: Mon, 8 Feb 2021 04:21:02 -0800 Subject: [PATCH 4/6] Roll forward up to cl/355902187. --- cpp/core/internal/base_pcp_handler.cc | 3 -- cpp/core/internal/base_pcp_handler_test.cc | 42 ++++++++++++++++++++-- 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/cpp/core/internal/base_pcp_handler.cc b/cpp/core/internal/base_pcp_handler.cc index cb4382d8..3bebcb01 100644 --- a/cpp/core/internal/base_pcp_handler.cc +++ b/cpp/core/internal/base_pcp_handler.cc @@ -247,7 +247,6 @@ void BasePcpHandler::OnEncryptionSuccessRunnable( ProcessPreConnectionInitiationFailure( endpoint_id, connection_info.channel.get(), {Status::kEndpointIoError}, connection_info.result.lock().get()); - connection_info.result.reset(); return; } @@ -308,7 +307,6 @@ void BasePcpHandler::OnEncryptionFailureRunnable( ProcessPreConnectionInitiationFailure(endpoint_id, info.channel.get(), {Status::kEndpointIoError}, info.result.lock().get()); - info.result.reset(); } Status BasePcpHandler::RequestConnection(ClientProxy* client, @@ -1010,7 +1008,6 @@ void BasePcpHandler::ProcessTieBreakLoss( ProcessPreConnectionInitiationFailure(endpoint_id, info->channel.get(), {Status::kEndpointIoError}, info->result.lock().get()); - info->result.reset(); ProcessPreConnectionResultFailure(client, endpoint_id); } diff --git a/cpp/core/internal/base_pcp_handler_test.cc b/cpp/core/internal/base_pcp_handler_test.cc index ae8b5da6..bbd48478 100644 --- a/cpp/core/internal/base_pcp_handler_test.cc +++ b/cpp/core/internal/base_pcp_handler_test.cc @@ -14,6 +14,7 @@ #include "core/params.h" #include "proto/connections/offline_wire_formats.pb.h" #include "platform/base/byte_array.h" +#include "platform/base/exception.h" #include "platform/public/count_down_latch.h" #include "platform/public/pipe.h" #include "proto/connections_enums.pb.h" @@ -58,6 +59,9 @@ class MockEndpointChannel : public BaseEndpointChannel { ExceptionOr DoRead() { return BaseEndpointChannel::Read(); } Exception DoWrite(const ByteArray& data) { + if (broken_write_) { + return {Exception::kFailed}; + } return BaseEndpointChannel::Write(data); } absl::Time DoGetLastReadTimestamp() { @@ -74,6 +78,8 @@ class MockEndpointChannel : public BaseEndpointChannel { MOCK_METHOD(void, Pause, (), (override)); MOCK_METHOD(void, Resume, (), (override)); MOCK_METHOD(absl::Time, GetLastReadTimestamp, (), (const override)); + + bool broken_write_{false}; }; class MockPcpHandler : public BasePcpHandler { @@ -297,7 +303,8 @@ class BasePcpHandlerTest MockEndpointChannel* channel_b, ClientProxy* client, MockPcpHandler* pcp_handler, proto::connections::Medium connect_medium, - std::atomic_int* flag = nullptr) { + std::atomic_int* flag = nullptr, + Status expected_result = {Status::kSuccess}) { ConnectionRequestInfo info{ .endpoint_info = ByteArray{"ABCD"}, .listener = connection_listener_, @@ -311,7 +318,9 @@ class BasePcpHandlerTest .WillRepeatedly(Return(true)); EXPECT_CALL(*pcp_handler, GetStrategy) .WillRepeatedly(Return(Strategy::kP2pCluster)); - EXPECT_CALL(mock_connection_listener_.initiated_cb, Call).Times(1); + if (expected_result == Status{Status::kSuccess}) { + EXPECT_CALL(mock_connection_listener_.initiated_cb, Call).Times(1); + } // Simulate successful discovery. auto encryption_runner = std::make_unique(); auto allowed_mediums = pcp_handler->GetDiscoveryMediums(client); @@ -351,7 +360,7 @@ class BasePcpHandlerTest } EXPECT_EQ( pcp_handler->RequestConnection(client, endpoint_id, info, options), - Status{Status::kSuccess}); + expected_result); NEARBY_LOG(INFO, "Stopping Encryption Runner"); } @@ -465,6 +474,33 @@ TEST_P(BasePcpHandlerTest, RequestConnectionChangesState) { pcp_handler.DisconnectFromEndpointManager(); } +TEST_P(BasePcpHandlerTest, IoError_RequestConnectionFails) { + std::string endpoint_id{"1234"}; + ClientProxy client; + Mediums m; + EndpointChannelManager ecm; + EndpointManager em(&ecm); + BwuManager bwu(m, em, ecm, {}, {}); + MockPcpHandler pcp_handler(&m, &em, &ecm, &bwu); + StartDiscovery(&client, &pcp_handler); + auto mediums = pcp_handler.GetDiscoveryMediums(&client); + auto connect_medium = mediums[mediums.size() - 1]; + auto channel_pair = SetupConnection(pipe_a_, pipe_b_, connect_medium); + auto& channel_a = channel_pair.first; + auto& channel_b = channel_pair.second; + EXPECT_CALL(*channel_a, CloseImpl).Times(AtLeast(1)); + EXPECT_CALL(*channel_b, CloseImpl).Times(AtLeast(1)); + channel_b->broken_write_ = true; + EXPECT_CALL(mock_connection_listener_.rejected_cb, Call).Times(AtLeast(0)); + RequestConnection(endpoint_id, std::move(channel_a), channel_b.get(), &client, + &pcp_handler, connect_medium, nullptr, + {Status::kEndpointIoError}); + NEARBY_LOG(INFO, "RequestConnection complete"); + channel_b->Close(); + bwu.Shutdown(); + pcp_handler.DisconnectFromEndpointManager(); +} + TEST_P(BasePcpHandlerTest, AcceptConnectionChangesState) { std::string endpoint_id{"1234"}; ClientProxy client; From 8876ceb8fb2974f397e4b5c5750aed13756896f8 Mon Sep 17 00:00:00 2001 From: hai007 Date: Mon, 8 Feb 2021 04:22:58 -0800 Subject: [PATCH 5/6] Roll forward up to cl/355961673. --- cpp/platform/public/bluetooth_classic.cc | 37 +++++++++---------- cpp/platform/public/bluetooth_classic_test.cc | 5 +++ 2 files changed, 23 insertions(+), 19 deletions(-) diff --git a/cpp/platform/public/bluetooth_classic.cc b/cpp/platform/public/bluetooth_classic.cc index a4fc80f0..af53b863 100644 --- a/cpp/platform/public/bluetooth_classic.cc +++ b/cpp/platform/public/bluetooth_classic.cc @@ -19,18 +19,12 @@ BluetoothSocket BluetoothClassicMedium::ConnectToService( } bool BluetoothClassicMedium::StartDiscovery(DiscoveryCallback callback) { - { - MutexLock lock(&mutex_); - if (discovery_enabled_) { - NEARBY_LOG(INFO, "BT Discovery already enabled; impl=%p", &GetImpl()); - return false; - } - discovery_callback_ = std::move(callback); - devices_.clear(); - discovery_enabled_ = true; - NEARBY_LOG(INFO, "BT Discovery enabled; impl=%p", &GetImpl()); + MutexLock lock(&mutex_); + if (discovery_enabled_) { + NEARBY_LOG(INFO, "BT Discovery already enabled; impl=%p", &GetImpl()); + return false; } - return impl_->StartDiscovery({ + bool success = impl_->StartDiscovery({ .device_discovered_cb = [this](api::BluetoothDevice& device) { MutexLock lock(&mutex_); @@ -68,17 +62,22 @@ bool BluetoothClassicMedium::StartDiscovery(DiscoveryCallback callback) { discovery_callback_.device_lost_cb(context.device); }, }); + if (success) { + discovery_callback_ = std::move(callback); + devices_.clear(); + discovery_enabled_ = true; + NEARBY_LOG(INFO, "BT Discovery enabled; impl=%p", &GetImpl()); + } + return success; } bool BluetoothClassicMedium::StopDiscovery() { - { - MutexLock lock(&mutex_); - if (!discovery_enabled_) return true; - discovery_enabled_ = false; - discovery_callback_ = {}; - devices_.clear(); - NEARBY_LOG(INFO, "BT Discovery disabled: impl=%p", &GetImpl()); - } + MutexLock lock(&mutex_); + if (!discovery_enabled_) return true; + discovery_enabled_ = false; + discovery_callback_ = {}; + devices_.clear(); + NEARBY_LOG(INFO, "BT Discovery disabled: impl=%p", &GetImpl()); return impl_->StopDiscovery(); } diff --git a/cpp/platform/public/bluetooth_classic_test.cc b/cpp/platform/public/bluetooth_classic_test.cc index f13322b6..b03e964c 100644 --- a/cpp/platform/public/bluetooth_classic_test.cc +++ b/cpp/platform/public/bluetooth_classic_test.cc @@ -269,6 +269,11 @@ TEST_F(BluetoothClassicMediumTest, CanListenForService) { server_socket.Close(); } +TEST_F(BluetoothClassicMediumTest, FailIfDiscovering) { + EXPECT_TRUE(bt_a_->StartDiscovery({})); + EXPECT_FALSE(bt_a_->StartDiscovery({})); +} + } // namespace } // namespace nearby } // namespace location From c2c8fbf1e33eceabf15dbb2974fb5cf09b06298a Mon Sep 17 00:00:00 2001 From: hai007 Date: Mon, 8 Feb 2021 04:24:26 -0800 Subject: [PATCH 6/6] Roll forward up to cl/355978895. --- cpp/core/internal/bwu_manager.cc | 49 +++++++++++++++++++++++++++++-- cpp/platform/base/feature_flags.h | 3 ++ 2 files changed, 49 insertions(+), 3 deletions(-) diff --git a/cpp/core/internal/bwu_manager.cc b/cpp/core/internal/bwu_manager.cc index 3582d78f..47742df4 100644 --- a/cpp/core/internal/bwu_manager.cc +++ b/cpp/core/internal/bwu_manager.cc @@ -9,6 +9,7 @@ #include "core/internal/webrtc_bwu_handler.h" #include "core/internal/wifi_lan_bwu_handler.h" #include "platform/base/byte_array.h" +#include "platform/base/feature_flags.h" #include "platform/public/count_down_latch.h" #include "proto/connections_enums.pb.h" #include "absl/functional/bind_front.h" @@ -122,6 +123,11 @@ void BwuManager::InitiateBwuForEndpoint(ClientProxy* client, if (in_progress_upgrades_.contains(endpoint_id)) { return; } + if (FeatureFlags::GetInstance() + .GetFlags() + .disallow_out_of_order_bwu_avail_event) { + CancelRetryUpgradeAlarm(endpoint_id); + } auto channel = channel_manager_->GetChannelForEndpoint(endpoint_id); @@ -297,9 +303,19 @@ void BwuManager::OnIncomingConnection( } const std::string& endpoint_id = introduction.endpoint_id(); - auto item = in_progress_upgrades_.extract(endpoint_id); - if (item.empty()) return; - ClientProxy* mapped_client = item.mapped(); + ClientProxy* mapped_client; + if (FeatureFlags::GetInstance() + .GetFlags() + .disallow_out_of_order_bwu_avail_event) { + const auto item = in_progress_upgrades_.find(endpoint_id); + if (item == in_progress_upgrades_.end()) return; + mapped_client = item->second; + } else { + auto item = in_progress_upgrades_.extract(endpoint_id); + if (item.empty()) return; + mapped_client = item.mapped(); + } + CancelRetryUpgradeAlarm(endpoint_id); if (mapped_client == nullptr) { // This was never a fully EstablishedConnection, no need to provide a @@ -363,11 +379,28 @@ void BwuManager::RunUpgradeProtocol( void BwuManager::ProcessBwuPathAvailableEvent( ClientProxy* client, const string& endpoint_id, const UpgradePathInfo& upgrade_path_info) { + if (in_progress_upgrades_.contains(endpoint_id)) { + NEARBY_LOG(INFO, "Invoking duplicate ProcessBwuPathAvailableEvent for %s", + endpoint_id.c_str()); + if (FeatureFlags::GetInstance() + .GetFlags() + .disallow_out_of_order_bwu_avail_event) { + NEARBY_LOG(WARNING, + "BandwidthUpgradeManager is ignoring bandwidth upgrade for " + "endpoint %s because we're already upgrading bandwidth for " + "that endpoint. Something may have gone wrong, as it seems " + "we're out of sync with the remote device.", + endpoint_id.c_str()); + return; + } + } + Medium medium = parser::UpgradePathInfoMediumToMedium(upgrade_path_info.medium()); if (medium_ == Medium::UNKNOWN_MEDIUM) { SetCurrentBwuHandler(medium); } + // Check for the correct medium so we don't process an incorrect OfflineFrame. if (medium != medium_) { RunUpgradeFailedProtocol(client, endpoint_id, upgrade_path_info); @@ -381,6 +414,11 @@ void BwuManager::ProcessBwuPathAvailableEvent( return; } + if (FeatureFlags::GetInstance() + .GetFlags() + .disallow_out_of_order_bwu_avail_event) { + in_progress_upgrades_.emplace(endpoint_id, client); + } RunUpgradeProtocol(client, endpoint_id, std::move(channel)); } @@ -642,6 +680,11 @@ void BwuManager::ProcessSafeToClosePriorChannelEvent( // Report the success to the client client->OnBandwidthChanged(endpoint_id, channel->GetMedium()); + if (FeatureFlags::GetInstance() + .GetFlags() + .disallow_out_of_order_bwu_avail_event) { + in_progress_upgrades_.erase(endpoint_id); + } } void BwuManager::ProcessUpgradeFailureEvent( diff --git a/cpp/platform/base/feature_flags.h b/cpp/platform/base/feature_flags.h index 7972190e..a6c90dc0 100644 --- a/cpp/platform/base/feature_flags.h +++ b/cpp/platform/base/feature_flags.h @@ -18,6 +18,9 @@ class FeatureFlags { // ServiceController is released to prevent calls to that API from // other threads. bool disable_released_service_controller = true; + // Ignore subsequent BWU Available events when we're still processing the + // first one. + bool disallow_out_of_order_bwu_avail_event = true; }; static const FeatureFlags& GetInstance() {