diff --git a/sharing/nearby_sharing_service_impl.cc b/sharing/nearby_sharing_service_impl.cc index fd404b73..99ef31c7 100644 --- a/sharing/nearby_sharing_service_impl.cc +++ b/sharing/nearby_sharing_service_impl.cc @@ -817,7 +817,8 @@ void NearbySharingServiceImpl::Accept( bool accept_success = incoming_session->AcceptTransfer( context_->GetClock(), absl::bind_front( - &NearbySharingServiceImpl::OnPayloadTransferUpdate, this)); + &NearbySharingServiceImpl::IncomingPayloadTransferUpdate, + this)); std::move(status_codes_callback)( accept_success ? StatusCodes::kOk : StatusCodes::kOutOfOrderApiCall); @@ -2947,7 +2948,7 @@ void NearbySharingServiceImpl::OnReceiveConnectionResponse( std::optional frame) { OnFrameRead(share_target_id, std::move(frame)); }, - absl::bind_front(&NearbySharingServiceImpl::OnPayloadTransferUpdate, + absl::bind_front(&NearbySharingServiceImpl::OutgoingPayloadTransferUpdate, this)); } @@ -2972,7 +2973,7 @@ void NearbySharingServiceImpl::OnStorageCheckCompleted( NL_LOG(INFO) << __func__ << ": Auto-accepting self share."; session.AcceptTransfer( context_->GetClock(), - absl::bind_front(&NearbySharingServiceImpl::OnPayloadTransferUpdate, + absl::bind_front(&NearbySharingServiceImpl::IncomingPayloadTransferUpdate, this)); OnTransferStarted(/*is_incoming=*/true); } @@ -3099,9 +3100,9 @@ std::optional NearbySharingServiceImpl::CreateShareTarget( return target; } -void NearbySharingServiceImpl::OnPayloadTransferUpdate( +void NearbySharingServiceImpl::IncomingPayloadTransferUpdate( int64_t share_target_id, TransferMetadata metadata) { - ShareSession* session = GetShareSession(share_target_id); + IncomingShareSession* session = GetIncomingShareSession(share_target_id); if (!session) { // ShareTarget already disconnected. NL_LOG(WARNING) @@ -3109,69 +3110,87 @@ void NearbySharingServiceImpl::OnPayloadTransferUpdate( << share_target_id; return; } - bool is_in_progress = - metadata.status() == TransferMetadata::Status::kInProgress; - // kInProgress status is logged extensively elsewhere so avoid the spam. - if (!is_in_progress) { + if (metadata.status() != TransferMetadata::Status::kInProgress) { NL_VLOG(1) << __func__ << ": Nearby Share service: " << "Payload transfer update for share target with ID " << share_target_id << ": " << TransferMetadata::StatusToString(metadata.status()); } - bool payload_incomplete = false; - if (session->IsIncoming()) { - IncomingShareSession* incoming_session = - GetIncomingShareSession(share_target_id); + // Update file paths during progress. It may impact transfer speed. + // TODO: b/289290115 - Revisit UpdateFilePath to enhance transfer speed for + // MacOS. + if (update_file_paths_in_progress_) { + session->UpdateFilePayloadPaths(); + } - // Update file paths during progress. It may impact transfer speed. - // TODO: b/289290115 - Revisit UpdateFilePath to enhance transfer speed for - // MacOS. - if (update_file_paths_in_progress_) { - incoming_session->UpdateFilePayloadPaths(); + if (metadata.status() == TransferMetadata::Status::kComplete) { + if (!session->FinalizePayloads()) { + metadata = TransferMetadataBuilder() + .set_status(TransferMetadata::Status::kIncompletePayloads) + .build(); } - if (metadata.status() == TransferMetadata::Status::kComplete) { - if (!incoming_session->FinalizePayloads()) { - payload_incomplete = true; - } - - fast_initiation_scanner_cooldown_timer_ = std::make_unique( - *service_thread_, "fast_initiation_scanner_cooldown_timer", - kFastInitiationScannerCooldown, [this]() { - fast_initiation_scanner_cooldown_timer_.reset(); - InvalidateFastInitiationScanning(); - }); - } else if (metadata.status() == TransferMetadata::Status::kCancelled) { - NL_VLOG(1) << __func__ << ": Update file paths for cancelled transfer"; - if (!update_file_paths_in_progress_) { - incoming_session->UpdateFilePayloadPaths(); - } + fast_initiation_scanner_cooldown_timer_ = std::make_unique( + *service_thread_, "fast_initiation_scanner_cooldown_timer", + kFastInitiationScannerCooldown, [this]() { + fast_initiation_scanner_cooldown_timer_.reset(); + InvalidateFastInitiationScanning(); + }); + } else if (metadata.status() == TransferMetadata::Status::kCancelled) { + NL_VLOG(1) << __func__ << ": Update file paths for cancelled transfer"; + if (!update_file_paths_in_progress_) { + session->UpdateFilePayloadPaths(); } } // Make sure to call this before calling Disconnect, or we risk losing some // transfer updates in the receive case due to the Disconnect call cleaning up // share targets. - session->UpdateTransferMetadata( - payload_incomplete - ? TransferMetadataBuilder() - .set_status(TransferMetadata::Status::kIncompletePayloads) - .build() - : metadata); + session->UpdateTransferMetadata(metadata); - if (payload_incomplete || - TransferMetadata::IsFinalStatus(metadata.status())) { - // final status already sent, no need to send again on disconnect. - session->set_disconnect_status(TransferMetadata::Status::kUnknown); + if (TransferMetadata::IsFinalStatus(metadata.status())) { + // Cancellation has its own disconnection strategy, possibly adding a + // delay before disconnection to provide the other party time to process + // the cancellation. + if (metadata.status() != TransferMetadata::Status::kCancelled) { + session->Disconnect(); + } } - // Cancellation has its own disconnection strategy, possibly adding a - // delay before disconnection to provide the other party time to process - // the cancellation. - if (TransferMetadata::IsFinalStatus(metadata.status()) && - metadata.status() != TransferMetadata::Status::kCancelled) { - Disconnect(share_target_id, metadata); +} + +void NearbySharingServiceImpl::OutgoingPayloadTransferUpdate( + int64_t share_target_id, TransferMetadata metadata) { + OutgoingShareSession* session = GetOutgoingShareSession(share_target_id); + if (!session) { + // ShareTarget already disconnected. + NL_LOG(WARNING) + << "Received payload update after share target disconnected: " + << share_target_id; + return; + } + + // kInProgress status is logged extensively elsewhere so avoid the spam. + if (metadata.status() != TransferMetadata::Status::kInProgress) { + NL_VLOG(1) << __func__ << ": Nearby Share service: " + << "Payload transfer update for share target with ID " + << share_target_id << ": " + << TransferMetadata::StatusToString(metadata.status()); + } + + // Make sure to call this before calling Disconnect, or we risk losing some + // transfer updates in the receive case due to the Disconnect call cleaning up + // share targets. + session->UpdateTransferMetadata(metadata); + + if (TransferMetadata::IsFinalStatus(metadata.status())) { + // Cancellation has its own disconnection strategy, possibly adding a + // delay before disconnection to provide the other party time to process + // the cancellation. + if (metadata.status() != TransferMetadata::Status::kCancelled) { + DisconnectOutgoing(*session, metadata); + } } } @@ -3198,40 +3217,18 @@ void NearbySharingServiceImpl::RemoveIncomingPayloads( file_handler_.DeleteFilesFromDisk(std::move(files_for_deletion), []() {}); } -void NearbySharingServiceImpl::Disconnect(int64_t share_target_id, - TransferMetadata metadata) { - ShareSession* session = GetShareSession(share_target_id); - if (!session) { - NL_LOG(WARNING) - << __func__ - << ": Failed to disconnect. No share session found for target - " - << share_target_id; - return; - } - - std::string endpoint_id = session->endpoint_id(); +void NearbySharingServiceImpl::DisconnectOutgoing(OutgoingShareSession& session, + TransferMetadata metadata) { + std::string endpoint_id = session.endpoint_id(); // Failed to send or receive. No point in continuing, so disconnect // immediately. if (metadata.status() != TransferMetadata::Status::kComplete) { - if (session->IsConnected()) { - session->connection()->Close(); - } else { - nearby_connections_manager_->Disconnect(endpoint_id); - } - return; - } - - // Files received successfully. Receivers can immediately cancel. - if (session->IsIncoming()) { - if (session->IsConnected()) { - session->connection()->Close(); - } else { - nearby_connections_manager_->Disconnect(endpoint_id); - } + session.Disconnect(); return; } // Disconnect after a timeout to make sure any pending payloads are sent. + // This can happen after the session has been destroyed. auto timer = std::make_unique( *service_thread_, "disconnection_timeout_alarm", kOutgoingDisconnectionDelay, [this, endpoint_id]() { @@ -3240,8 +3237,6 @@ void NearbySharingServiceImpl::Disconnect(int64_t share_target_id, }); disconnection_timeout_alarms_[endpoint_id] = std::move(timer); - - session->set_disconnect_status(TransferMetadata::Status::kUnknown); } IncomingShareSession& NearbySharingServiceImpl::CreateIncomingShareSession( diff --git a/sharing/nearby_sharing_service_impl.h b/sharing/nearby_sharing_service_impl.h index f0862e38..e623d53d 100644 --- a/sharing/nearby_sharing_service_impl.h +++ b/sharing/nearby_sharing_service_impl.h @@ -367,10 +367,14 @@ class NearbySharingServiceImpl const std::optional& certificate, bool is_incoming); - void OnPayloadTransferUpdate(int64_t share_target_id, - TransferMetadata metadata); + void IncomingPayloadTransferUpdate( + int64_t share_target_id, TransferMetadata metadata); + void OutgoingPayloadTransferUpdate( + int64_t share_target_id, TransferMetadata metadata); + void RemoveIncomingPayloads(const IncomingShareSession& session); - void Disconnect(int64_t share_target_id, TransferMetadata metadata); + void DisconnectOutgoing(OutgoingShareSession& session, + TransferMetadata metadata); IncomingShareSession& CreateIncomingShareSession( const ShareTarget& share_target, absl::string_view endpoint_id, diff --git a/sharing/share_session.cc b/sharing/share_session.cc index 335b37bd..1bfea722 100644 --- a/sharing/share_session.cc +++ b/sharing/share_session.cc @@ -131,6 +131,14 @@ bool ShareSession::OnConnected(const NearbySharingDecoder& decoder, return true; } +void ShareSession::Disconnect() { + if (connection_ == nullptr) { + return; + } + // Do not clear connection_ here. It will be cleared in OnDisconnect(). + connection_->Close(); +} + void ShareSession::Abort(TransferMetadata::Status status) { NL_DCHECK(TransferMetadata::IsFinalStatus(status)) << "Abort should only be called with a final status"; diff --git a/sharing/share_session.h b/sharing/share_session.h index ceaf4982..5cbfe8f6 100644 --- a/sharing/share_session.h +++ b/sharing/share_session.h @@ -144,6 +144,8 @@ class ShareSession { void SetTokenForTests(std::string token) { token_ = std::move(token); } + void Disconnect(); + protected: virtual void InvokeTransferUpdateCallback( const TransferMetadata& metadata) = 0; diff --git a/sharing/share_session_test.cc b/sharing/share_session_test.cc index a90a70f4..a85dd878 100644 --- a/sharing/share_session_test.cc +++ b/sharing/share_session_test.cc @@ -441,5 +441,29 @@ TEST(ShareSessionTest, AbortConnected) { EXPECT_TRUE(disconnected); } +TEST(ShareSessionTest, Disconnect) { + FakeNearbyConnectionsManager connections_manager; + NearbySharingDecoderImpl nearby_sharing_decoder; + ShareTarget share_target; + TestShareSession session(std::string(kEndpointId), share_target); + FakeNearbyConnection connection; + bool disconnected = false; + connection.SetDisconnectionListener( + [&disconnected]() { disconnected = true; }); + EXPECT_TRUE(session.OnConnected(nearby_sharing_decoder, absl::Now(), + &connections_manager, &connection)); + + session.Disconnect(); + + EXPECT_TRUE(disconnected); +} + +TEST(ShareSessionTest, DisconnectNotConnected) { + ShareTarget share_target; + TestShareSession session(std::string(kEndpointId), share_target); + + session.Disconnect(); +} + } // namespace } // namespace nearby::sharing