From 2e5491275cf809f9bfa6705dcceb827b5af78c68 Mon Sep 17 00:00:00 2001 From: Francis Tsui Date: Wed, 18 Dec 2024 13:43:52 -0800 Subject: [PATCH] Move disconnection timeout into OutgoingShareSession. PiperOrigin-RevId: 707655553 --- sharing/nearby_sharing_service_impl.cc | 57 ++++++++------------------ sharing/nearby_sharing_service_impl.h | 5 --- sharing/outgoing_share_session.cc | 16 ++++---- sharing/outgoing_share_session.h | 14 ++++--- sharing/outgoing_share_session_test.cc | 13 +++--- 5 files changed, 42 insertions(+), 63 deletions(-) diff --git a/sharing/nearby_sharing_service_impl.cc b/sharing/nearby_sharing_service_impl.cc index 9fb44791b1..8794578281 100644 --- a/sharing/nearby_sharing_service_impl.cc +++ b/sharing/nearby_sharing_service_impl.cc @@ -346,8 +346,6 @@ void NearbySharingServiceImpl::Cleanup() { last_outgoing_metadata_.reset(); locally_cancelled_share_target_ids_.clear(); - disconnection_timeout_alarms_.clear(); - is_scanning_ = false; is_transferring_ = false; is_receiving_files_ = false; @@ -510,14 +508,11 @@ void NearbySharingServiceImpl::RegisterSendSurface( // Let newly registered send surface catch up with discovered share // targets from current scanning session. - // if (is_scanning_) { - for (const auto& item : outgoing_share_target_map_) { - LOG(INFO) << "Reporting discovered target " - << item.second.ToString() - << " when registering send surface"; - wrapped_callback.OnShareTargetDiscovered(item.second); - } - // } + for (const auto& item : outgoing_share_target_map_) { + LOG(INFO) << "Reporting discovered target " << item.second.ToString() + << " when registering send surface"; + wrapped_callback.OnShareTargetDiscovered(item.second); + } // Set Share Start time for Foreground Send Surfaces if (state == SendSurfaceState::kForeground) { @@ -818,8 +813,8 @@ void NearbySharingServiceImpl::Accept( GetIncomingShareSession(share_target_id); if (incoming_session != nullptr) { // Incoming session. - bool accept_success = incoming_session->AcceptTransfer( - absl::bind_front( + bool accept_success = + incoming_session->AcceptTransfer(absl::bind_front( &NearbySharingServiceImpl::OnIncomingPayloadTransferUpdates, this, share_target_id)); std::move(status_codes_callback)( @@ -1747,7 +1742,6 @@ void NearbySharingServiceImpl::OnOutgoingDecryptedCertificate( absl::string_view endpoint_id, absl::Span endpoint_info, const Advertisement& advertisement, std::optional certificate) { - // The certificate provides the device name, in order to create a ShareTarget // to represent this remote device. std::optional share_target = @@ -1776,7 +1770,7 @@ void NearbySharingServiceImpl::OnOutgoingDecryptedCertificate( } if (FindDuplicateInOutgoingShareTargets(endpoint_id, *share_target)) { DeduplicateInOutgoingShareTarget(*share_target, endpoint_id, - std::move(certificate)); + std::move(certificate)); FinishEndpointDiscoveryEvent(); return; } @@ -2872,10 +2866,9 @@ void NearbySharingServiceImpl::OnStorageCheckCompleted( } // Don't need to wait for user to accept for Self share. LOG(INFO) << __func__ << ": Auto-accepting self share."; - session.AcceptTransfer( - absl::bind_front( - &NearbySharingServiceImpl::OnIncomingPayloadTransferUpdates, this, - session.share_target().id)); + session.AcceptTransfer(absl::bind_front( + &NearbySharingServiceImpl::OnIncomingPayloadTransferUpdates, this, + session.share_target().id)); OnTransferStarted(/*is_incoming=*/true); } @@ -3004,9 +2997,8 @@ void NearbySharingServiceImpl::OnIncomingPayloadTransferUpdates( } if (metadata->status() == TransferMetadata::Status::kIncompletePayloads) { - OnIncomingFilesMetadataUpdated(share_target_id, - std::move(*metadata), - /*success=*/false); + OnIncomingFilesMetadataUpdated(share_target_id, std::move(*metadata), + /*success=*/false); return; } if (metadata->status() == TransferMetadata::Status::kComplete) { @@ -3091,24 +3083,8 @@ void NearbySharingServiceImpl::OnOutgoingPayloadTransferUpdates( << TransferMetadata::StatusToString(metadata->status()); } - // When kComplete is received from PayloadTracker, we need to wait for - // receiver to close connection before we know that the transfer has - // succeeded. Here we delay the kComplete update until receiver disconnects. - // A 1 min timer is setup so that if we do not receive disconnect from - // receiver, we assume the transfer has failed. if (metadata->status() == TransferMetadata::Status::kComplete) { - session->DelayCompleteMetadata(*metadata); - auto timer = std::make_unique( - *service_thread_, "disconnection_timeout_alarm", - kOutgoingDisconnectionDelay, - [this, share_target_id = session->share_target().id]() { - OutgoingShareSession* session = - GetOutgoingShareSession(share_target_id); - if (session != nullptr) { - session->DisconnectionTimeout(); - } - }); - disconnection_timeout_alarms_[session->endpoint_id()] = std::move(timer); + session->DelayComplete(*metadata); return; } // Make sure to call this before calling Disconnect, or we risk losing some @@ -3293,8 +3269,7 @@ std::optional NearbySharingServiceImpl::RemoveOutgoingShareTargetWithEndpointId( absl::string_view endpoint_id) { VLOG(1) << __func__ << ":Outgoing connection to " << endpoint_id - << " disconnected, cancel disconnection timer"; - disconnection_timeout_alarms_.erase(endpoint_id); + << " disconnected"; auto target_node = outgoing_share_target_map_.extract(endpoint_id); if (target_node.empty()) { LOG(WARNING) << __func__ << ": endpoint_id=" << endpoint_id @@ -3373,7 +3348,7 @@ void NearbySharingServiceImpl::MoveToDiscoveryCache(std::string endpoint_id, auto [it, inserted] = discovery_cache_.insert_or_assign(endpoint_id, std::move(cache_entry)); LOG(INFO) << "[Dedupped] added to discovery_cache: " << endpoint_id << " by " - << (inserted ? "insert" : "assign"); + << (inserted ? "insert" : "assign"); } void NearbySharingServiceImpl::CreateOutgoingShareSession( diff --git a/sharing/nearby_sharing_service_impl.h b/sharing/nearby_sharing_service_impl.h index dba4ae9e7c..b65e6a39a0 100644 --- a/sharing/nearby_sharing_service_impl.h +++ b/sharing/nearby_sharing_service_impl.h @@ -543,11 +543,6 @@ class NearbySharingServiceImpl // unnecessary backend API call. absl::flat_hash_set discovered_advertisements_retried_set_; - // A map of ShareTarget id to disconnection timeout callback. Used to only - // disconnect after a timeout to keep sending any pending payloads. - absl::flat_hash_map> - disconnection_timeout_alarms_; - // The current advertising power level. PowerLevel::kUnknown while not // advertising. PowerLevel advertising_power_level_ = PowerLevel::kUnknown; diff --git a/sharing/outgoing_share_session.cc b/sharing/outgoing_share_session.cc index 401b24b3f9..d11bd3308a 100644 --- a/sharing/outgoing_share_session.cc +++ b/sharing/outgoing_share_session.cc @@ -467,7 +467,7 @@ std::optional OutgoingShareSession::ExtractNextPayload() { return std::nullopt; } -void OutgoingShareSession::DelayCompleteMetadata( +void OutgoingShareSession::DelayComplete( const TransferMetadata& complete_metadata) { LOG(INFO) << "Delay complete notification until receiver disconnects for target " @@ -478,12 +478,14 @@ void OutgoingShareSession::DelayCompleteMetadata( TransferMetadataBuilder::Clone(complete_metadata); builder.set_status(TransferMetadata::Status::kInProgress); UpdateTransferMetadata(builder.build()); -} - -void OutgoingShareSession::DisconnectionTimeout() { - VLOG(1) << "Disconnection delay timeed out for target " << share_target().id; - pending_complete_metadata_.reset(); - Disconnect(); + disconnection_timeout_ = std::make_unique( + service_thread(), "disconnection_timeout_alarm", + kOutgoingDisconnectionDelay, [this]() { + VLOG(1) << "Disconnection delay timed out for target " + << share_target().id; + pending_complete_metadata_.reset(); + Disconnect(); + }); } void OutgoingShareSession::UpdateSessionForDedup( diff --git a/sharing/outgoing_share_session.h b/sharing/outgoing_share_session.h index 6d5157e99a..59d890c980 100644 --- a/sharing/outgoing_share_session.h +++ b/sharing/outgoing_share_session.h @@ -130,11 +130,13 @@ class OutgoingShareSession : public ShareSession { // Called when all payloads have been sent. void SendAttachmentsCompleted(const TransferMetadata& metadata); - // Cache the kComplete metadata in pending_complete_metadata_ and forward a - // modified copy that changes kComplete into kInProgress. - void DelayCompleteMetadata(const TransferMetadata& complete_metadata); - // Disconnect timeout expired without receiving client disconnect. - void DisconnectionTimeout(); + // Wait for receiver to close connection before we notify that the transfer + // has succeeded. Here we delay the `complete_metadata` update until receiver + // disconnects and forward a modified copy that changes kComplete into + // kInProgress. + // A 1 min timer is setup so that if we do not receive disconnect from + // receiver, we assume the transfer has failed. + void DelayComplete(const TransferMetadata& complete_metadata); // Used only for OutgoingShareSession De-duplication. void UpdateSessionForDedup( const ShareTarget& share_target, @@ -183,6 +185,8 @@ class OutgoingShareSession : public ShareSession { std::unique_ptr mutual_acceptance_timeout_; std::optional pending_complete_metadata_; absl::Time connection_start_time_; + // Timeout waiting for remote disconnect in order to complete transfer. + std::unique_ptr disconnection_timeout_; }; } // namespace nearby::sharing diff --git a/sharing/outgoing_share_session_test.cc b/sharing/outgoing_share_session_test.cc index 07069331af..bcae2a8f46 100644 --- a/sharing/outgoing_share_session_test.cc +++ b/sharing/outgoing_share_session_test.cc @@ -815,7 +815,7 @@ TEST_F(OutgoingShareSessionTest, ProcessKeyVerificationResultSuccess) { EXPECT_THAT(session_.os_type(), Eq(OSType::WINDOWS)); } -TEST_F(OutgoingShareSessionTest, DelayCompleteMetadataReceiverDisconnect) { +TEST_F(OutgoingShareSessionTest, DelayCompleteReceiverDisconnect) { NearbyConnectionImpl connection(device_info_); session_.set_session_id(1234); ConnectionSuccess(&connection); @@ -826,14 +826,14 @@ TEST_F(OutgoingShareSessionTest, DelayCompleteMetadataReceiverDisconnect) { EXPECT_CALL(transfer_metadata_callback_, Call(_, HasStatus(TransferMetadata::Status::kInProgress))); - session_.DelayCompleteMetadata(complete_metadata); + session_.DelayComplete(complete_metadata); EXPECT_CALL(transfer_metadata_callback_, Call(_, HasStatus(TransferMetadata::Status::kComplete))); session_.OnDisconnect(); } -TEST_F(OutgoingShareSessionTest, DelayCompleteMetadataDisconnectTimeout) { +TEST_F(OutgoingShareSessionTest, DelayCompleteDisconnectTimeout) { NearbyConnectionImpl connection(device_info_); session_.set_session_id(1234); std::vector endpoint_info = {1, 2, 3, 4}; @@ -854,9 +854,12 @@ TEST_F(OutgoingShareSessionTest, DelayCompleteMetadataDisconnectTimeout) { EXPECT_CALL(transfer_metadata_callback_, Call(_, HasStatus(TransferMetadata::Status::kInProgress))); - session_.DelayCompleteMetadata(complete_metadata); + session_.DelayComplete(complete_metadata); + // Fast forward to the disconnection timeout. + fake_clock_.FastForward(absl::Seconds(60)); + fake_task_runner_.SyncWithTimeout(absl::Milliseconds(100)); - session_.DisconnectionTimeout(); + // session_.DisconnectionTimeout(); // Verify that connection is closed. EXPECT_THAT( connections_manager_.connection_endpoint_info(kEndpointId).has_value(),