From aee56f07296359fbb478032efb2558ea142a551b Mon Sep 17 00:00:00 2001 From: Juliet Levesque Date: Thu, 15 Jun 2023 11:06:24 -0700 Subject: [PATCH] [Nearby Connections] Add CancellationFlag::Uncancel Add CancellationFlag::Uncancel and on calls to ClientProxy::AddCancellationFlag, if a flag is already in the map, uncancel it. This will address the case when users use NC to share/receive a file, then cancel in the middle because the wrong file was selected, and then re-do right after. Without this change, the second share/receive process will be seen as cancelled with cancellation flags enabled. However this change will uncancel the flag which is added in RequestConnection and OnConnectionInitiated in the NS flow. PiperOrigin-RevId: 540633470 --- connections/implementation/client_proxy.cc | 9 +++++ .../implementation/client_proxy_test.cc | 37 +++++++++++++++++++ internal/platform/cancellation_flag.cc | 13 +++++++ internal/platform/cancellation_flag.h | 9 +++++ 4 files changed, 68 insertions(+) diff --git a/connections/implementation/client_proxy.cc b/connections/implementation/client_proxy.cc index 7e3dfa1e..99637d39 100644 --- a/connections/implementation/client_proxy.cc +++ b/connections/implementation/client_proxy.cc @@ -634,6 +634,15 @@ void ClientProxy::AddCancellationFlag(const std::string& endpoint_id) { auto item = cancellation_flags_.find(endpoint_id); if (item != cancellation_flags_.end()) { + // A new flag may be added to the map with the same endpoint, even if a + // flag already in the map has already been cancelled, when an endpoint + // is being reused (for example, the case when users use NS to share/receive + // a file, then cancel in the middle because the wrong file was selected + // and then re-do right after). The flag needs to be uncancelled in order + // to support a new attempt with the same endpoint. + if (item->second->Cancelled()) { + item->second->Uncancel(); + } return; } cancellation_flags_.emplace(endpoint_id, diff --git a/connections/implementation/client_proxy_test.cc b/connections/implementation/client_proxy_test.cc index ed728f0c..10ef6260 100644 --- a/connections/implementation/client_proxy_test.cc +++ b/connections/implementation/client_proxy_test.cc @@ -965,6 +965,43 @@ TEST_F(ClientProxyTest, SetRemoteInfoCorrect) { OsInfo::ANDROID); } +// Test ClientProxy::AddCancellationFlag, where if a flag is already in the map, +// uncancel it. This addresses the case when users use NS to share/receive a +// file, then cancel in the middle because the wrong file was selected, and then +// re-do right after. Without the ability to uncancel a flag in +// `AddCancelationFlag`, the second share/receive process will +// be seen as cancelled with cancellation flags enabled. However this tests that +// it will uncancel the flag which is added in RequestConnection and +// OnConnectionInitiated in the NS flow, and allow another attempt with the +// same endpoint. +TEST_F(ClientProxyTest, UncancelCancellationFlags) { + // Enable cancellation flags. + MediumEnvironment::Instance().SetFeatureFlags(kTestCases[0]); + Endpoint advertising_endpoint = + StartAdvertising(&client1_, advertising_connection_listener_); + + // Add a cancellation flag to the client proxy. + client1_.AddCancellationFlag(advertising_endpoint.id); + auto flag = client1_.GetCancellationFlag(advertising_endpoint.id); + EXPECT_FALSE(flag->Cancelled()); + EXPECT_FALSE( + client1_.GetCancellationFlag(advertising_endpoint.id)->Cancelled()); + + // Cancel the flag. + flag->Cancel(); + EXPECT_TRUE(flag->Cancelled()); + EXPECT_TRUE( + client1_.GetCancellationFlag(advertising_endpoint.id)->Cancelled()); + + // On subsequent calls to add a new cancellation flag, expect an the flag to + // be uncancelled. + client1_.AddCancellationFlag(advertising_endpoint.id); + flag = client1_.GetCancellationFlag(advertising_endpoint.id); + EXPECT_FALSE(flag->Cancelled()); + EXPECT_FALSE( + client1_.GetCancellationFlag(advertising_endpoint.id)->Cancelled()); +} + TEST_F(ClientProxyTest, GetLocalDeviceWorksWithoutDeviceProvider) { auto device = client1_.GetLocalDevice(); EXPECT_NE(device, nullptr); diff --git a/internal/platform/cancellation_flag.cc b/internal/platform/cancellation_flag.cc index f9daa1ed..2836efa6 100644 --- a/internal/platform/cancellation_flag.cc +++ b/internal/platform/cancellation_flag.cc @@ -55,6 +55,19 @@ void CancellationFlag::Cancel() { } } +void CancellationFlag::Uncancel() { + // Return immediately as no-op if feature flag is not enabled. + if (!FeatureFlags::GetInstance().GetFlags().enable_cancellation_flag) { + return; + } + + { + absl::MutexLock lock(mutex_.get()); + assert(cancelled_); + cancelled_ = false; + } +} + bool CancellationFlag::Cancelled() const { absl::MutexLock lock(mutex_.get()); diff --git a/internal/platform/cancellation_flag.h b/internal/platform/cancellation_flag.h index 41185c29..9a8b7280 100644 --- a/internal/platform/cancellation_flag.h +++ b/internal/platform/cancellation_flag.h @@ -41,6 +41,15 @@ class CancellationFlag { // Set the flag as cancelled. void Cancel() ABSL_LOCKS_EXCLUDED(mutex_); + // Set the flag as uncancelled. This is needed for the case where the same + // endpoint is being reused, and callers want to reset the cancellation flag + // for the new attempt. Without this API, the reused endpoint will + // use the cancelled flag. + // + // It is expected that calleers will only call `Uncancel()` if the flag is + // already cancelled (check via `Cancelled()`). + void Uncancel() ABSL_LOCKS_EXCLUDED(mutex_); + // Returns true if the flag has been set to cancelled. bool Cancelled() const ABSL_LOCKS_EXCLUDED(mutex_);