From 361f45f5df710d44f2b9af78ee738f836ef80461 Mon Sep 17 00:00:00 2001 From: Francis Tsui Date: Fri, 26 Sep 2025 18:34:28 -0700 Subject: [PATCH] More cleanup of OutgoingTargetsManager. PiperOrigin-RevId: 812032769 --- sharing/outgoing_share_session.cc | 2 + sharing/outgoing_targets_manager.cc | 238 ++++++++++++++-------------- sharing/outgoing_targets_manager.h | 28 ++-- 3 files changed, 136 insertions(+), 132 deletions(-) diff --git a/sharing/outgoing_share_session.cc b/sharing/outgoing_share_session.cc index 9b662928..82cce224 100644 --- a/sharing/outgoing_share_session.cc +++ b/sharing/outgoing_share_session.cc @@ -499,6 +499,8 @@ bool OutgoingShareSession::UpdateSessionForDedup( << "Share target id cannot be changed during deduplication."; set_share_target(share_target); if (IsConnected()) { + LOG(INFO) << __func__ << ": session for share_target.id=" << share_target.id + << " is connected, not updating."; return false; } set_endpoint_id(endpoint_id); diff --git a/sharing/outgoing_targets_manager.cc b/sharing/outgoing_targets_manager.cc index b9e473f5..1d7d0959 100644 --- a/sharing/outgoing_targets_manager.cc +++ b/sharing/outgoing_targets_manager.cc @@ -16,6 +16,7 @@ #include +#include #include #include #include @@ -68,48 +69,39 @@ OutgoingTargetsManager::OutgoingTargetsManager( void OutgoingTargetsManager::OnShareTargetDiscovered( ShareTarget share_target, absl::string_view endpoint_id, std::optional certificate) { - if (FindDuplicateInOutgoingShareTargets(endpoint_id, share_target)) { - if (DeduplicateInOutgoingShareTarget(share_target, endpoint_id, - std::move(certificate))) { - share_target_updated_callback_(share_target); - + std::optional old_id = + UpdateExistingTarget(endpoint_id, share_target); + if (old_id.has_value()) { + // Reusing existing share target id. + share_target.id = *old_id; + if (UpdateExistingSession(share_target, endpoint_id, + std::move(certificate))) { LOG(INFO) << __func__ - << ": [Dedupped] NotifyShareTargetUpdated to all surfaces " - "for share_target: " - << share_target.ToString(); + << ": ShareTarget updated, endpoint_id: " << endpoint_id + << ", share_target: " << share_target.ToString(); + share_target_updated_callback_(share_target); } return; } - bool in_discovery_cache = - FindDuplicateInDiscoveryCache(endpoint_id, share_target); - VLOG(1) << __func__ << ": Adding (endpoint_id=" << endpoint_id - << ", share_target_id=" << share_target.id - << ") to outgoing share target map"; - CreateOutgoingShareSession(share_target, endpoint_id, std::move(certificate)); + old_id = FindInDiscoveryCache(endpoint_id, share_target); + bool in_discovery_cache = old_id.has_value(); + if (in_discovery_cache) { + share_target.id = *old_id; + } + LOG(INFO) << __func__ + << (in_discovery_cache ? ": Recovered from discovery cache" + : ": Discovered new target") + << ": endpoint_id=" << endpoint_id + << ", share_target=" << share_target.ToString(); + AddTarget(share_target, endpoint_id, std::move(certificate)); if (in_discovery_cache) { share_target_updated_callback_(share_target); - - LOG(INFO) - << __func__ - << ": [Dedupped] Reported NotifyShareTargetUpdated to all surfaces " - "for share_target: " - << share_target.ToString(); - return; + } else { + share_target_discovered_callback_(share_target); } - // Update the endpoint id for the share target. - LOG(INFO) << __func__ << ": An endpoint: " << endpoint_id - << " has been discovered, with an advertisement " - "containing a valid share target with id: " - << share_target.id; - - share_target_discovered_callback_(share_target); - - VLOG(1) << __func__ << ": NotifyShareTargetDiscovered: share_target: " - << share_target.ToString() << " endpoint_id=" << endpoint_id - << " to all send surfaces."; } -bool OutgoingTargetsManager::DeduplicateInOutgoingShareTarget( +bool OutgoingTargetsManager::UpdateExistingSession( const ShareTarget& share_target, absl::string_view endpoint_id, std::optional certificate) { // TODO(b/343764269): may need to update last_outgoing_metadata_ if the @@ -119,105 +111,100 @@ bool OutgoingTargetsManager::DeduplicateInOutgoingShareTarget( auto session_it = outgoing_share_session_map_.find(share_target.id); if (session_it == outgoing_share_session_map_.end()) { - LOG(WARNING) << __func__ << ": share_target.id=" << share_target.id - << " not found in outgoing share session map."; + LOG(ERROR) << __func__ << ": share_target.id=" << share_target.id + << " not found in outgoing share session map."; return false; } - if (session_it->second.IsConnected()) { - LOG(INFO) << __func__ << ": share_target.id=" << share_target.id - << " is connected, not updating outgoing_share_session_map_."; - return false; - } - session_it->second.UpdateSessionForDedup(share_target, std::move(certificate), - endpoint_id); - return true; + return session_it->second.UpdateSessionForDedup( + share_target, std::move(certificate), endpoint_id); } -bool OutgoingTargetsManager::FindDuplicateInDiscoveryCache( - absl::string_view endpoint_id, ShareTarget& share_target) { +std::optional OutgoingTargetsManager::FindInDiscoveryCache( + absl::string_view endpoint_id, const ShareTarget& share_target) { auto it = discovery_cache_.find(endpoint_id); if (it != discovery_cache_.end()) { + int64_t old_id = it->second.share_target.id; // If endpoint info changes for an endpoint ID, NC will send a rediscovery // event for the same endpoint id. - LOG(INFO) << __func__ - << ": [Dedupped] Found duplicate endpoint_id: " << endpoint_id - << ", share_target.id changed from: " << share_target.id << " to " - << it->second.share_target.id; - share_target.id = it->second.share_target.id; + LOG(INFO) << __func__ << ": Found existing endpoint_id: " << endpoint_id + << ", mapping share_target.id: " << share_target.id + << " to: " << it->second.share_target.id; discovery_cache_.erase(it); - return true; + return old_id; } - for (auto it = discovery_cache_.begin(); it != discovery_cache_.end(); ++it) { - if (it->second.share_target.device_id == share_target.device_id) { - LOG(INFO) << __func__ - << ": [Dedupped] Found duplicate device_id, share_target.id " - "changed from: " - << share_target.id << " to " << it->second.share_target.id - << ". New endpoint_id: " << endpoint_id; - // Share targets in discovery cache have receive_disabled set to true. - // Copy only the id field from cache entry, - share_target.id = it->second.share_target.id; - discovery_cache_.erase(it); - return true; - } + auto device_id_it = std::find_if( + discovery_cache_.begin(), discovery_cache_.end(), + [&share_target](const auto& pair) { + return pair.second.share_target.device_id == share_target.device_id; + }); + if (device_id_it != discovery_cache_.end()) { + int64_t old_id = device_id_it->second.share_target.id; + LOG(INFO) << __func__ + << ": Found existing device_id, updating endpoint ID: " + << device_id_it->first << " to: " << endpoint_id + << " , mapping share_target.id: " << share_target.id + << " to: " << old_id; + discovery_cache_.erase(device_id_it); + return old_id; } - return false; + return std::nullopt; } -bool OutgoingTargetsManager::FindDuplicateInOutgoingShareTargets( - absl::string_view endpoint_id, ShareTarget& share_target) { +std::optional OutgoingTargetsManager::UpdateExistingTarget( + absl::string_view endpoint_id, const ShareTarget& share_target) { // If the duplicate is found, share_target.id needs to be updated to the old // "discovered" share_target_id so NotifyShareTargetUpdated matches a target // that was discovered before. auto it = outgoing_share_target_map_.find(endpoint_id); if (it != outgoing_share_target_map_.end()) { + int64_t old_share_target_id = it->second.id; // If endpoint info changes for an endpoint ID, NC will send a rediscovery // event for the same endpoint id. - LOG(INFO) << __func__ - << ": [Dedupped] Found duplicate endpoint_id: " << endpoint_id - << " in outgoing_share_target_map, share_target.id changed from: " - << share_target.id << " to " << it->second.id; - share_target.id = it->second.id; + LOG(INFO) << __func__ << ": Found existing endpoint_id: " << endpoint_id + << ", mapping share_target.id: " << share_target.id + << " to: " << old_share_target_id; it->second = share_target; - return true; + it->second.id = old_share_target_id; + return old_share_target_id; } - for (auto it = outgoing_share_target_map_.begin(); - it != outgoing_share_target_map_.end(); ++it) { - if (it->second.device_id == share_target.device_id) { - LOG(INFO) - << __func__ - << ": [Dedupped] Found duplicate device_id, endpoint ID " - "changed from: " - << it->first << " to " << endpoint_id - << " in outgoing_share_target_map, share_target.id changed from: " - << share_target.id << " to " << it->second.id; - share_target.id = it->second.id; - outgoing_share_target_map_.erase(it); - outgoing_share_target_map_.insert_or_assign(endpoint_id, share_target); - return true; - } + auto device_id_it = std::find_if( + outgoing_share_target_map_.begin(), outgoing_share_target_map_.end(), + [&share_target](const auto& pair) { + return pair.second.device_id == share_target.device_id; + }); + if (device_id_it != outgoing_share_target_map_.end()) { + int64_t old_share_target_id = device_id_it->second.id; + LOG(INFO) << __func__ + << ": Found existing device_id, updating endpoint ID: " + << device_id_it->first << " to: " << endpoint_id + << " , mapping share_target.id: " << share_target.id + << " to: " << old_share_target_id; + ShareTarget new_share_target = share_target; + new_share_target.id = old_share_target_id; + outgoing_share_target_map_.erase(device_id_it); + outgoing_share_target_map_.insert( + {std::string(endpoint_id), new_share_target}); + return old_share_target_id; } - return false; + return std::nullopt; } -std::optional -OutgoingTargetsManager::RemoveOutgoingShareTargetWithEndpointId( +std::optional OutgoingTargetsManager::RemoveTarget( absl::string_view endpoint_id) { - VLOG(1) << __func__ << ":Outgoing connection to " << endpoint_id - << " disconnected"; + VLOG(1) << __func__ << ":Removing endpoint_id " << endpoint_id; auto target_node = outgoing_share_target_map_.extract(endpoint_id); if (target_node.empty()) { LOG(WARNING) << __func__ << ": endpoint_id=" << endpoint_id - << " not found in outgoing share target map."; + << " not found."; return std::nullopt; } ShareTarget& share_target = target_node.mapped(); - VLOG(1) << __func__ << ": Removing (endpoint_id=" << endpoint_id - << ", share_target.id=" << target_node.mapped().id - << ") from outgoing share target map"; + VLOG(1) << __func__ + << ": Removing share_target.id=" << target_node.mapped().id + << " from outgoing share target map"; // Do not destroy the session until it has been removed from the map. // Session destruction can trigger callbacks that traverses the map and it @@ -239,8 +226,7 @@ OutgoingTargetsManager::RemoveOutgoingShareTargetWithEndpointId( // This prevents the endpoint_id from being invalidated in this function. void OutgoingTargetsManager::OnShareTargetLost(std::string endpoint_id, absl::Duration retention) { - std::optional share_target_opt = - RemoveOutgoingShareTargetWithEndpointId(endpoint_id); + std::optional share_target_opt = RemoveTarget(endpoint_id); if (!share_target_opt.has_value()) { return; } @@ -258,38 +244,54 @@ void OutgoingTargetsManager::OnShareTargetLost(std::string endpoint_id, return; } ShareTarget& share_target = cache_node.mapped().share_target; - LOG(INFO) << ": Removing (endpoint_id=" << endpoint_id - << ", share_target.id=" << share_target.id - << ") from discovery_cache after " << retention; - + LOG(INFO) << ": ShareTarget lost after retention: " << retention + << ", endpoint_id=" << endpoint_id + << ", share_target=" << share_target.ToString(); share_target_lost_callback_(share_target); }); // Send ShareTarget update to set receive disabled to true. + LOG(INFO) << __func__ + << ": ShareTarget disabled, endpoint_id: " << endpoint_id << ", " + << cache_entry.share_target.ToString(); share_target_updated_callback_(cache_entry.share_target); - 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"); + discovery_cache_.insert_or_assign(endpoint_id, std::move(cache_entry)); } -void OutgoingTargetsManager::CreateOutgoingShareSession( +void OutgoingTargetsManager::AddTarget( const ShareTarget& share_target, absl::string_view endpoint_id, std::optional certificate) { - outgoing_share_target_map_.insert_or_assign(endpoint_id, share_target); - auto [it_out, inserted] = outgoing_share_session_map_.try_emplace( + auto [target_it, target_inserted] = outgoing_share_target_map_.insert( + {std::string(endpoint_id), share_target}); + if (!target_inserted) { + if (target_it->second.id != share_target.id) { + LOG(ERROR) << __func__ << ": endpoint_id=" << endpoint_id + << " already associated with share_target id=" + << target_it->second.id + << ", cannot replace with share_target id=" << share_target.id; + return; + } + LOG(WARNING) << __func__ << ": endpoint_id=" << endpoint_id + << " already exists, share target not updated."; + } + auto [session_it, session_inserted] = outgoing_share_session_map_.try_emplace( share_target.id, &clock_, service_thread_, &connections_manager_, analytics_recorder_, std::string(endpoint_id), share_target, transfer_update_callback_); - if (!inserted) { + if (!session_inserted) { LOG(WARNING) << __func__ << ": share_target.id=" << share_target.id << " already exists in outgoing share session map. This " "should NOT happen"; - } else { - auto& session = it_out->second; - if (certificate.has_value()) { - session.set_certificate(std::move(*certificate)); + if (session_it->second.IsConnected()) { + LOG(WARNING) << __func__ + << ": session for share_target.id=" << share_target.id + << " is connected, certificate not updated."; + return; } } + auto& session = session_it->second; + if (certificate.has_value()) { + session.set_certificate(std::move(*certificate)); + } } OutgoingShareSession* OutgoingTargetsManager::GetOutgoingShareSession( @@ -313,10 +315,10 @@ void OutgoingTargetsManager::AllTargetsLost(absl::Duration retention) { void OutgoingTargetsManager::Cleanup() { while (!outgoing_share_target_map_.empty()) { - // Latch endpoint_id here since RemoveOutgoingShareTargetWithEndpointId() - // will remove the entry from the map. + // Latch endpoint_id here since RemoveTarget() will remove the entry from + // the map. std::string endpoint_id = outgoing_share_target_map_.begin()->first; - RemoveOutgoingShareTargetWithEndpointId(endpoint_id); + RemoveTarget(endpoint_id); } discovery_cache_.clear(); } diff --git a/sharing/outgoing_targets_manager.h b/sharing/outgoing_targets_manager.h index 5ddf0115..e37f6df8 100644 --- a/sharing/outgoing_targets_manager.h +++ b/sharing/outgoing_targets_manager.h @@ -95,32 +95,32 @@ class OutgoingTargetsManager { absl::AnyInvocable callback); private: - // Looks for a duplicate of the share target in the outgoing share - // target map. The share target's id is changed to match an existing target if - // available. Returns true if the duplicate is found. - bool FindDuplicateInOutgoingShareTargets(absl::string_view endpoint_id, - ShareTarget& share_target); + // If an existing target matching either endpoint_id or share_target.device_id + // is found, the share target is updated and the existing share target id is + // returned. + // Otherwise, std::nullopt is returned. + std::optional UpdateExistingTarget(absl::string_view endpoint_id, + const ShareTarget& share_target); // Update the entry in outgoing_share_session_map_ with the new share target. // Returns true if the share target was updated. - bool DeduplicateInOutgoingShareTarget( + bool UpdateExistingSession( const ShareTarget& share_target, absl::string_view endpoint_id, std::optional certificate); - // Looks for a duplicate of the share target in the discovery cache. + // Looks for existing share target in the discovery cache. // If found, the share target is removed from the discovery cache and its - // id is copied into `share_target`. - // Returns true if the duplicate is found. - bool FindDuplicateInDiscoveryCache(absl::string_view endpoint_id, - ShareTarget& share_target); + // id is returned. + // Otherwise, std::nullopt is returned. + std::optional FindInDiscoveryCache(absl::string_view endpoint_id, + const ShareTarget& share_target); - void CreateOutgoingShareSession( + void AddTarget( const ShareTarget& share_target, absl::string_view endpoint_id, std::optional certificate); // Returns the share target if it has been removed, std::nullopt otherwise. - std::optional RemoveOutgoingShareTargetWithEndpointId( - absl::string_view endpoint_id); + std::optional RemoveTarget(absl::string_view endpoint_id); // Cache a recently lost share target to be re-discovered. // Purged after expiry_timer.