feat(iroh-net): add `MagicEndpoint::conn_type_stream` returns a stream that reports connection type changes for a `node_id` #2161

ramfox · 2024-04-09T03:52:36Z

Description

MagicEndpoint::conn_type_stream returns a Stream that reports changes for a magicsock::Endpoint with a given node_id in a magicsock::NodeMap.

It will error if no address information for that node_id exists in the NodeMap.

This PR also adjusts the Endpoint::info() method to use the same ConnectionType that gets reported to the stream.

Change checklist

Self-review.
Documentation updates if relevant.
Tests if relevant.

flub · 2024-04-10T14:48:25Z

iroh-net/src/magic_endpoint.rs

+    async fn magic_endpoint_wait_for_direct_conn() {
+        let _logging_guard = iroh_test::logging::setup();
+        let (relay_map, relay_url, _relay_guard) = run_relay_server().await.unwrap();
+        let mut rng = rand_chacha::ChaCha8Rng::seed_from_u64(42);


this is a very specific rng. I have no opinions on it, but would love to learn why?

🤣 TBH, this was taken from another test & I didn't change it. I'm assuming it was originally 42 because 42 is the answer to life the universe and everything

iroh-net/src/magic_endpoint.rs

flub · 2024-04-10T14:58:43Z

iroh-net/src/magic_endpoint.rs

+        eprintln!("node id 2 {ep2_nodeid}");
+
+        let res_ep1 = tokio::spawn(handle_direct_conn(ep1.clone(), ep2_nodeid));
+        let res_ep2 = tokio::spawn(handle_direct_conn(ep2.clone(), ep1_nodeid));


if you want to be really careful you get an abort handle of these and put a call to them in a iroh_test::CallOnDrop so that your tasks are always cleaned up.

flub · 2024-04-10T15:04:28Z

iroh-net/src/magicsock/node_map/endpoint.rs

+    /// It is possible we will never have a viable `best_addr`, in which case
+    /// no alert will ever be issued.
+    pub fn notify_has_best_addr(&mut self) -> oneshot::Receiver<()> {
+        self.assign_best_addr_from_candidates_if_empty();


Do you really want to call this from here? I'm kind of -1 because it is logic that influences how the magicsock behaves but is called from a place that's not in the normal flow. The magicsock will already call this from somewhere else when it thinks this is needed.

good point. if we go back to that flow, I'll remove it.

flub · 2024-04-10T15:06:27Z

sorry, meant to review this before you woke up but it got lost in an open tab...

ramfox · 2024-04-11T04:08:28Z

iroh-net/src/magicsock/node_map/endpoint.rs

@@ -171,6 +174,7 @@ impl Endpoint {
            direct_addr_state: BTreeMap::new(),
            last_used: options.active.then(Instant::now),
            last_call_me_maybe: None,
+            conn_type: Watchable::new(ConnectionType::None),


my biggest concern with this approach is how "heavy" it might be to have a Watchable for each node_map::Endpoint

It does acquire an additional RwLock write handle for each time you set it. Same would be for a tokio Watch. Though I think there would be very little contention for this, so it's probably fine?

iroh-net/src/magic_endpoint.rs

iroh-net/src/magicsock/node_map.rs

flub · 2024-04-11T08:35:44Z

iroh-net/src/magicsock/node_map/endpoint.rs

@@ -171,6 +174,7 @@ impl Endpoint {
            direct_addr_state: BTreeMap::new(),
            last_used: options.active.then(Instant::now),
            last_call_me_maybe: None,
+            conn_type: Watchable::new(ConnectionType::None),


It does acquire an additional RwLock write handle for each time you set it. Same would be for a tokio Watch. Though I think there would be very little contention for this, so it's probably fine?

This blocking method will return once a direct connection has been established between the endpoint and the node at `node_id`, or return in error if a timeout occurs. If no `Endpoint` exists for this `node_id` in the `NodeMap`, one will be created. This can be called before or after `connect`. If a direct connection already exists between the endpoint and the node at `node_id` when this method is called, it will immediately return.

…tils::run_relay_server`

… creating an `EndpointInfo`

rklaehn mentioned this pull request Apr 9, 2024

iroh-net: wait until home derper is known #2162

Closed

ramfox force-pushed the ramfox/alert-first-holepunch branch from 9b095c6 to fe42335 Compare April 10, 2024 01:30

ramfox changed the title ~~WIP feat(iroh-net): add MagicEndpoint::wait_until_direct_conn~~ feat(iroh-net): add MagicEndpoint::wait_until_direct_conn Apr 10, 2024

ramfox marked this pull request as ready for review April 10, 2024 03:03

ramfox requested review from flub and dignifiedquire April 10, 2024 03:04

dignifiedquire added this to the v0.14.0 milestone Apr 10, 2024

flub reviewed Apr 10, 2024

View reviewed changes

ramfox force-pushed the ramfox/alert-first-holepunch branch from 5dea7b2 to 4ad3c78 Compare April 11, 2024 03:53

ramfox commented Apr 11, 2024

View reviewed changes

ramfox force-pushed the ramfox/alert-first-holepunch branch from 4ad3c78 to 016aa09 Compare April 11, 2024 04:09

ramfox requested a review from flub April 11, 2024 04:11

flub approved these changes Apr 11, 2024

View reviewed changes

ramfox self-assigned this Apr 11, 2024

ramfox added 7 commits April 11, 2024 13:52

MagicEndpoint::wait_for_direct_conn takes and optional timeout

f5a0195

use test relay

6afaa83

why is this test failing with a local relay server?

9be8457

need to use insecure_skip_relay_cert_verify for tests using `test_u…

d476984

…tils::run_relay_server`

refactor to return a stream of ConnectionType changes

905bbef

adjust Endpoint::info() to use the Endpoint::conn_type field when…

eb6044d

… creating an `EndpointInfo`

ramfox force-pushed the ramfox/alert-first-holepunch branch from b8f4e8e to eb6044d Compare April 11, 2024 17:52

ramfox changed the title ~~feat(iroh-net): add MagicEndpoint::wait_until_direct_conn~~ feat(iroh-net): add MagicEndpoint::conn_type_stream returns a stream that reports connection type changes for a node_id Apr 11, 2024

fix doc comments

d4983e8

ramfox force-pushed the ramfox/alert-first-holepunch branch from 0a08dd5 to d4983e8 Compare April 11, 2024 18:28

ramfox added this pull request to the merge queue Apr 11, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 11, 2024

ramfox added this pull request to the merge queue Apr 12, 2024

Merged via the queue into main with commit 7986394 Apr 12, 2024
20 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(iroh-net): add `MagicEndpoint::conn_type_stream` returns a stream that reports connection type changes for a `node_id` #2161

feat(iroh-net): add `MagicEndpoint::conn_type_stream` returns a stream that reports connection type changes for a `node_id` #2161

ramfox commented Apr 9, 2024 •

edited

Loading

flub Apr 10, 2024

ramfox Apr 11, 2024

flub Apr 10, 2024

flub Apr 10, 2024

ramfox Apr 11, 2024

flub commented Apr 10, 2024

ramfox Apr 11, 2024

flub Apr 11, 2024

flub Apr 11, 2024

feat(iroh-net): add MagicEndpoint::conn_type_stream returns a stream that reports connection type changes for a node_id #2161

feat(iroh-net): add MagicEndpoint::conn_type_stream returns a stream that reports connection type changes for a node_id #2161

Conversation

ramfox commented Apr 9, 2024 • edited Loading

Description

Change checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flub commented Apr 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(iroh-net): add `MagicEndpoint::conn_type_stream` returns a stream that reports connection type changes for a `node_id` #2161

feat(iroh-net): add `MagicEndpoint::conn_type_stream` returns a stream that reports connection type changes for a `node_id` #2161

ramfox commented Apr 9, 2024 •

edited

Loading