From 1d3b17821bfff84e27baa58d690e954d95ea7ccf Mon Sep 17 00:00:00 2001 From: Francis Tsui Date: Wed, 12 Feb 2025 16:07:36 -0800 Subject: [PATCH] Fix issue with multiple Cancellations for same target ID. PiperOrigin-RevId: 726234081 --- sharing/fake_nearby_sharing_service.cc | 7 ------- sharing/fake_nearby_sharing_service.h | 4 ---- sharing/nearby_sharing_service.h | 4 ---- sharing/nearby_sharing_service_impl.cc | 19 +++++-------------- sharing/nearby_sharing_service_impl.h | 3 --- sharing/share_session.cc | 8 +++++++- sharing/share_session.h | 4 +++- sharing/share_session_test.cc | 5 ++++- 8 files changed, 19 insertions(+), 35 deletions(-) diff --git a/sharing/fake_nearby_sharing_service.cc b/sharing/fake_nearby_sharing_service.cc index c4c96c2c..6516762a 100644 --- a/sharing/fake_nearby_sharing_service.cc +++ b/sharing/fake_nearby_sharing_service.cc @@ -150,13 +150,6 @@ void FakeNearbySharingService::Cancel( status_codes_callback(StatusCodes::kOk); } -// Returns true if the local user cancelled the transfer to remote -// |share_target|. -bool FakeNearbySharingService::DidLocalUserCancelTransfer( - int64_t share_target_id) { - return false; -} - std::string FakeNearbySharingService::Dump() const { return ""; } NearbyShareSettings* FakeNearbySharingService::GetSettings() { return nullptr; } diff --git a/sharing/fake_nearby_sharing_service.h b/sharing/fake_nearby_sharing_service.h index 52deef61..9bfd6dbf 100644 --- a/sharing/fake_nearby_sharing_service.h +++ b/sharing/fake_nearby_sharing_service.h @@ -105,10 +105,6 @@ class FakeNearbySharingService : public NearbySharingService { std::function status_codes_callback) override; - // Returns true if the local user cancelled the transfer to remote - // |share_target|. - bool DidLocalUserCancelTransfer(int64_t share_target_id) override; - std::string Dump() const override; NearbyShareSettings* GetSettings() override; diff --git a/sharing/nearby_sharing_service.h b/sharing/nearby_sharing_service.h index 1fc5cdef..2f05441d 100644 --- a/sharing/nearby_sharing_service.h +++ b/sharing/nearby_sharing_service.h @@ -227,10 +227,6 @@ class NearbySharingService { int64_t share_target_id, std::function status_codes_callback) = 0; - // Returns true if the local user cancelled the transfer to remote - // |share_target|. - virtual bool DidLocalUserCancelTransfer(int64_t share_target_id) = 0; - // Checks to make sure visibility setting is valid and updates the service's // visibility if so. virtual void SetVisibility( diff --git a/sharing/nearby_sharing_service_impl.cc b/sharing/nearby_sharing_service_impl.cc index 5dfbc502..4a0bb3f1 100644 --- a/sharing/nearby_sharing_service_impl.cc +++ b/sharing/nearby_sharing_service_impl.cc @@ -344,7 +344,6 @@ void NearbySharingServiceImpl::Cleanup() { last_incoming_metadata_.reset(); last_outgoing_metadata_.reset(); - locally_cancelled_share_target_ids_.clear(); is_scanning_ = false; is_transferring_ = false; @@ -890,12 +889,6 @@ void NearbySharingServiceImpl::Cancel( [this, share_target_id, status_codes_callback = std::move(status_codes_callback)]() { LOG(INFO) << __func__ << ": User canceled transfer"; - if (locally_cancelled_share_target_ids_.contains(share_target_id)) { - LOG(WARNING) << __func__ << ": Cancel is called again."; - status_codes_callback(StatusCodes::kOutOfOrderApiCall); - return; - } - locally_cancelled_share_target_ids_.insert(share_target_id); DoCancel(share_target_id, std::move(status_codes_callback), /*is_initiator_of_cancellation=*/true); }); @@ -922,7 +915,11 @@ void NearbySharingServiceImpl::DoCancel( // cancellation signals. Also, note that there might not be any ongoing // payload transfer, for example, if a connection has not been established // yet. - session->CancelPayloads(); + if (!session->CancelPayloads()) { + // If session has already been cancelled, report success. + status_codes_callback(StatusCodes::kOk); + return; + } // Inform the user that the transfer has been cancelled before disconnecting // because subsequent disconnections might be interpreted as failure. @@ -969,12 +966,6 @@ void NearbySharingServiceImpl::DoCancel( std::move(status_codes_callback)(StatusCodes::kOk); } -bool NearbySharingServiceImpl::DidLocalUserCancelTransfer( - int64_t share_target_id) { - return absl::c_linear_search(locally_cancelled_share_target_ids_, - share_target_id); -} - void NearbySharingServiceImpl::SetVisibility( proto::DeviceVisibility visibility, absl::Duration expiration, absl::AnyInvocable callback) { diff --git a/sharing/nearby_sharing_service_impl.h b/sharing/nearby_sharing_service_impl.h index 285effe5..5c0e758e 100644 --- a/sharing/nearby_sharing_service_impl.h +++ b/sharing/nearby_sharing_service_impl.h @@ -157,7 +157,6 @@ class NearbySharingServiceImpl void Cancel(int64_t share_target_id, std::function status_codes_callback) override; - bool DidLocalUserCancelTransfer(int64_t share_target_id) override; void SetVisibility( proto::DeviceVisibility visibility, absl::Duration expiration, absl::AnyInvocable callback) override; @@ -531,8 +530,6 @@ class NearbySharingServiceImpl // A map of Endpoint id to DiscoveryCacheEntry. // All ShareTargets in discovery cache have received_disabled set to true. absl::flat_hash_map discovery_cache_; - // The IDs of ShareTargets that we cancelled the transfer to. - absl::flat_hash_set locally_cancelled_share_target_ids_; // A map from endpoint ID to endpoint info from discovered, contact-based // advertisements that could not decrypt any available public certificates. // During discovery, if certificates are downloaded, we revisit this map and diff --git a/sharing/share_session.cc b/sharing/share_session.cc index 52b7a76b..8c737406 100644 --- a/sharing/share_session.cc +++ b/sharing/share_session.cc @@ -211,10 +211,16 @@ void ShareSession::SetAttachmentPayloadId(int64_t attachment_id, attachment_payload_map_[attachment_id] = payload_id; } -void ShareSession::CancelPayloads() { +bool ShareSession::CancelPayloads() { + if (is_cancelled_) { + LOG(INFO) << __func__ << ": Share session is already cancelled."; + return false; + } for (const auto& [attachment_id, payload_id] : attachment_payload_map_) { connections_manager_.Cancel(payload_id); } + is_cancelled_ = true; + return true; } void ShareSession::WriteFrame(const Frame& frame) { diff --git a/sharing/share_session.h b/sharing/share_session.h index 0a29d863..9a4f5816 100644 --- a/sharing/share_session.h +++ b/sharing/share_session.h @@ -119,7 +119,8 @@ class ShareSession { return attachment_container_; } - void CancelPayloads(); + // Returns false if session is already cancelled, otherwise returns true. + bool CancelPayloads(); const absl::flat_hash_map& attachment_payload_map() const { return attachment_payload_map_; @@ -215,6 +216,7 @@ class ShareSession { AttachmentContainer attachment_container_; absl::flat_hash_map attachment_payload_map_; PayloadTracker::PayloadUpdateQueue* payload_updates_queue_ = nullptr; + bool is_cancelled_ = false; }; } // namespace nearby::sharing diff --git a/sharing/share_session_test.cc b/sharing/share_session_test.cc index 4c892144..53469e41 100644 --- a/sharing/share_session_test.cc +++ b/sharing/share_session_test.cc @@ -264,10 +264,13 @@ TEST(ShareSessionTest, CancelPayloads) { session.SetAttachmentPayloadId(1, 2); session.SetAttachmentPayloadId(3, 4); - session.CancelPayloads(); + EXPECT_TRUE(session.CancelPayloads()); EXPECT_TRUE(session.connections_manager().WasPayloadCanceled(2)); EXPECT_TRUE(session.connections_manager().WasPayloadCanceled(4)); + + // Repeated calls to CancelPayloads returns false. + EXPECT_FALSE(session.CancelPayloads()); } TEST(ShareSessionTest, WriteResponseFrame) {