From f2fc08659453e7c4110c80c065ad17cbddf41686 Mon Sep 17 00:00:00 2001
From: Nick Mathewson <nickm@torproject.org>
Date: Thu, 22 Jun 2023 09:23:42 -0400
Subject: [PATCH] proto: Add ClientCirc::path_ref(), deprecate path().

The new path_ref() method returns an Arc<Path>, which gives a much
better API for reasons discussed in the new documentation of path().

(We could just replace path() if we'd prefer, but IMO having
path_ref() here isn't so bad.)
---
 crates/tor-proto/src/circuit.rs | 48 ++++++++++++++++++++++++---------
 1 file changed, 35 insertions(+), 13 deletions(-)

diff --git a/crates/tor-proto/src/circuit.rs b/crates/tor-proto/src/circuit.rs
index 294cf45d2..ec8574ed5 100644
--- a/crates/tor-proto/src/circuit.rs
+++ b/crates/tor-proto/src/circuit.rs
@@ -251,15 +251,13 @@ impl ClientCirc {
 
     /// Return a description of all the hops in this circuit.
     ///
-    /// Note that this method performs a deep copy over the `OwnedCircTarget`
-    /// values in the path.  This is undesirable for some applications; see
-    /// [ticket #787](https://gitlab.torproject.org/tpo/core/arti/-/issues/787).
+    /// This method is **deprecated** for several reasons:
+    ///   * It performs a deep copy.
+    ///   * It ignores virtual hops.
+    ///   * It's not so extensible.
     ///
-    /// Note also that this method ignores virtual hops.  This is likely to
-    /// change in the future, but it will require a backward-incompatible change
-    /// on the return type.
-    ///
-    /// TODO HS: Fix the virtual-hop issue.
+    /// Use [`ClientCirc::path_ref()`] instead.
+    #[deprecated(since = "0.11.1", note = "Use path_ref() instead.")]
     pub fn path(&self) -> Vec<OwnedChanTarget> {
         #[allow(clippy::unnecessary_filter_map)] // clippy is blind to the cfg
         self.mutable
@@ -276,6 +274,14 @@ impl ClientCirc {
             .collect()
     }
 
+    /// Return a [`Path`] object describing all the hops in this circuit.
+    ///
+    /// Note that this `Path` is not automatically updated if the circuit is
+    /// extended.
+    pub fn path_ref(&self) -> Arc<Path> {
+        self.mutable.lock().expect("poisoned_lock").path.clone()
+    }
+
     /// Return a reference to the channel that this circuit is connected to.
     ///
     /// A client circuit is always connected to some relay via a [`Channel`].
@@ -1332,11 +1338,27 @@ mod test {
             assert_eq!(circ.n_hops(), 4);
 
             // Do the path accessors report a reasonable outcome?
-            let path = circ.path();
-            assert_eq!(path.len(), 4);
-            use tor_linkspec::HasRelayIds;
-            assert_eq!(path[3].ed_identity(), example_target().ed_identity());
-            assert_ne!(path[0].ed_identity(), example_target().ed_identity());
+            #[allow(deprecated)]
+            {
+                let path = circ.path();
+                assert_eq!(path.len(), 4);
+                use tor_linkspec::HasRelayIds;
+                assert_eq!(path[3].ed_identity(), example_target().ed_identity());
+                assert_ne!(path[0].ed_identity(), example_target().ed_identity());
+            }
+            {
+                let path = circ.path_ref();
+                assert_eq!(path.n_hops(), 4);
+                use tor_linkspec::HasRelayIds;
+                assert_eq!(
+                    path.hops()[3].as_chan_target().unwrap().ed_identity(),
+                    example_target().ed_identity()
+                );
+                assert_ne!(
+                    path.hops()[0].as_chan_target().unwrap().ed_identity(),
+                    example_target().ed_identity()
+                );
+            }
         });
     }