feat!: introduce iroh-router crate #2832

dignifiedquire · 2024-10-23T18:24:39Z

The general goal is to have a single place to deal with custom protocols.

Breaking Changes

moved
- iroh::node::ProtocolHandler -> iroh::router::ProtocolHandler

github-actions · 2024-10-23T18:27:27Z

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2832/docs/iroh/

Last updated: 2024-10-24T11:52:11Z

github-actions · 2024-10-23T18:32:23Z

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 2255227

rklaehn · 2024-10-24T07:35:35Z

As an extraction of the router api this looks fine.

We can bikeshed the router API, but I guess that is for the next PR.

Generated the router docs, and it is a very nice small API. However, I don't like that getting a protocol handler can fail. We could change the API to return a typed handle so you can be sure that the get succeeds. However, that would make the builder API slightly more annoying.

Also, should we allow adding removing ALPNs dynamically?

rklaehn · 2024-10-24T07:45:58Z

Alternative to the add/remove stuff.

add(alpn: Vec, proto: ...) returns a handle. The handle is not cloneable. Get takes a &handle, remove - if we ever do it - takes a handle. The handle is a newtype for an u64.

Upside: get can be made to return a Thing instead of an Option. You can mount the same protocol multiple times under different ALPNs (perfectly valid IMHO).

Downside: complicates the builder API since you need to return the handle and the new version of the builder, or do a mutable builder API.

dignifiedquire · 2024-10-24T09:18:25Z

There will be quite a few cases, eg with dependent protocols, where you will not have a handle available, (without very annoying handling) and in the ffi world this becomes even more painful, so I don't think these handles are worth it.

dignifiedquire · 2024-10-24T09:19:14Z

The biggest "decision" I made, is currently the endpoint is constructed on its own, rather than by the RouterBuilder. This makes this PR simpler, but we probably want to change that down the line 🤷 to be more like the current NodeBuilder.

rklaehn · 2024-10-24T09:35:53Z

There will be quite a few cases, eg with dependent protocols, where you will not have a handle available, (without very annoying handling) and in the ffi world this becomes even more painful, so I don't think these handles are worth it.

You can just pass in the handle when constructing the protocol. It's just an int64 newtype.

Regarding ffi I don't know how that looks, so I can't properly answer. Can you describe the problem?

Anyway, as I mentioned this discussion is for later.

rklaehn · 2024-10-24T09:37:08Z

The biggest "decision" I made, is currently the endpoint is constructed on its own, rather than by the RouterBuilder. This makes this PR simpler, but we probably want to change that down the line 🤷 to be more like the current NodeBuilder.

I don't hate it, especially now that iroh-router is a separte crate.

iroh-router/Cargo.toml

flub · 2024-10-24T08:35:12Z

iroh-router/examples/custom-protocol.rs

+            // By calling stopped we wait until the remote iroh Endpoint has acknowledged
+            // all data.  This does not mean the remote application has received all data
+            // from the Endpoint.
+            send.stopped().await?;


I don't think this is helpful. This handles the entire connection, so this should be waiting until the remote closes the connection:

// Wait until the remote closes the connection, which it does once it // received the response. connection.closed().await;

this is copied from the other custom-protocol example, so no idea

Yeah, let's improve our examples!

flub · 2024-10-24T08:39:57Z

iroh-router/examples/custom-protocol.rs

+        // By calling stopped we wait until the remote iroh Endpoint has acknowledged all
+        // data.  This does not mean the remote application has received all data from the
+        // Endpoint.
+        send.stopped().await?;


I don't think adding this is helpful. The protocol requires a response anyway, you should go straight to reading that response. Adding this does no harm but I think it confuses people, we should keep examples as minimal to be correct.

flub · 2024-10-24T08:40:47Z

iroh-router/examples/custom-protocol.rs

+            Err(err) => return Err(err.into()),
+            Ok(_) => {}
+        };
+        // Upcast the raw bytes to the `Hash` type.


You're casting to a u64, not to Hash

iroh-router/examples/custom-protocol.rs

iroh-router/src/router.rs

flub · 2024-10-24T09:16:28Z

iroh-router/src/router.rs

+                                }
+                            }
+                            Some(Ok(Err(inner))) => {
+                                debug!("Task errored: {inner:?}");


This is an anyhow::Error I think, should use alt formatting for those when logging: {inner:#}.

Possibly the others are also better at alt formatting? But I'm not sure, would have to try it. For the Task panicked we certainly want to make sure to log the backtrace.

flub · 2024-10-24T09:25:29Z

iroh-router/src/router.rs

+        let cancel = CancellationToken::new();
+        let cancel_token = cancel.clone();
+
+        let fut = async move {


Suggested change

let fut = async move {

let run_loop_fut = async move {

flub · 2024-10-24T09:38:13Z

iroh-router/src/router.rs

+        endpoint
+            .clone()
+            .close(error_code.into(), b"provider terminating"),
+        // Shutdown protocol handlers.
+        protocols.shutdown(),


By doing this in a join you're making it a bit non-deterministic whether the protocol handlers will still have an endpoint when their .shutdown() is being called.

I'm in two minds on whether the router should be closing the endpoint (given the earlier API/commen of how you pass a clone of the endpoint to the router which makes it seem like you still own it). If it does however I think it might be best to do this at the very end once all other tasks from this router are stopped. This would give protocols the possibility of doing a much cleaner shutdown themselves. Maybe a protocol still wants to exchange some messages with the peer saying "i'm going to close now" or something similar.

this is the exact some shutdown routine that we currently have in the iroh node, I am okay in changing it if we think this is needed, but lets do this separately please

Agreed that we shouldn't change this in this PR, but want to comment on this:
Is this where we should implement a endpoint.all_closed().await instead?

flub · 2024-10-24T09:41:12Z

iroh/src/node.rs

@@ -112,10 +110,10 @@ pub struct Node<D> {
    // So we need
    // - `Shared` so we can `task.await` from all `Node` clones
    // - `MapErr` to map the `JoinError` to a `String`, because `JoinError` is `!Clone`
-    // - `AbortOnDropHandle` to make sure that the `task` is cancelled when all `Node`s are dropped
+    // - `AbortOnDropHandle` to make sure that the `task` is cancelled when all `Node`s are droppedo


flub · 2024-10-24T10:31:51Z

If the goal is just to move code around, I'm fine with this. But I do think the router has to figure out some bits that I bring up before 1.0.

Co-authored-by: Floris Bruynooghe <flub@n0.computer>

dignifiedquire · 2024-10-24T11:01:04Z

But I do think the router has to figure out some bits that I bring up before 1.0.

yes, this is not meant as any form of final API

flub · 2024-10-24T11:35:05Z

Cargo.toml

@@ -7,7 +7,7 @@ members = [
  "iroh-net",
  "iroh-test",
  "iroh-net/bench",
-  "iroh-cli",
+  "iroh-cli", "iroh-router",


formatting is weird btw

thanks cargo ...

flub · 2024-10-24T11:38:50Z

iroh-router/src/router.rs

+    // - `Shared` so we can `task.await` from all `Node` clones
+    // - `MapErr` to map the `JoinError` to a `String`, because `JoinError` is `!Clone`
+    // - `AbortOnDropHandle` to make sure that the `task` is cancelled when all `Node`s are dropped
+    //   (`Shared` acts like an `Arc` around its inner future).


which would prevent shutdown from being called as it consumes self...

The general goal is to have a single place to deal with custom protocols. ## Breaking Changes - moved - `iroh::node::ProtocolHandler` -> `iroh::router::ProtocolHandler` --------- Co-authored-by: Floris Bruynooghe <flub@n0.computer>

flub reviewed Oct 24, 2024

View reviewed changes

dignifiedquire marked this pull request as ready for review October 24, 2024 09:48

dignifiedquire and others added 5 commits October 24, 2024 12:54

refactor: extract protocolhandler trait into iroh-router

2a08e7d

refactor: extract custom protocol logic into iroh-router

d871e23

fixups

fa3a835

fixup import

9eeee23

Update iroh-router/src/router.rs

01de8db

Co-authored-by: Floris Bruynooghe <flub@n0.computer>

dignifiedquire force-pushed the feat-router branch from 6c277b2 to 01de8db Compare October 24, 2024 11:01

dignifiedquire added 2 commits October 24, 2024 13:06

apply CR

14dc6ab

fixup: typo

77b4fed

flub approved these changes Oct 24, 2024

View reviewed changes

fixup formatting

39ac431

dignifiedquire changed the title ~~feat: introduce iroh-router crate~~ feat!: introduce iroh-router crate Oct 24, 2024

dignifiedquire added this pull request to the merge queue Oct 25, 2024

Merged via the queue into main with commit 8f75005 Oct 25, 2024
27 of 29 checks passed

dignifiedquire deleted the feat-router branch October 25, 2024 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: introduce iroh-router crate #2832

feat!: introduce iroh-router crate #2832

dignifiedquire commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024 •

edited

Loading

rklaehn commented Oct 24, 2024

rklaehn commented Oct 24, 2024

dignifiedquire commented Oct 24, 2024

dignifiedquire commented Oct 24, 2024

rklaehn commented Oct 24, 2024

rklaehn commented Oct 24, 2024

flub Oct 24, 2024

dignifiedquire Oct 24, 2024

flub Oct 24, 2024

dignifiedquire Oct 24, 2024

flub Oct 24, 2024

dignifiedquire Oct 24, 2024

dignifiedquire Oct 24, 2024

flub Oct 24, 2024

dignifiedquire Oct 24, 2024

dignifiedquire Oct 24, 2024

flub Oct 24, 2024

flub Oct 24, 2024

dignifiedquire Oct 24, 2024

flub Oct 24, 2024

dignifiedquire Oct 24, 2024

matheus23 Oct 25, 2024

flub Oct 24, 2024

dignifiedquire Oct 24, 2024

flub commented Oct 24, 2024

dignifiedquire commented Oct 24, 2024

flub Oct 24, 2024

dignifiedquire Oct 24, 2024

flub Oct 24, 2024

feat!: introduce iroh-router crate #2832

feat!: introduce iroh-router crate #2832

Conversation

dignifiedquire commented Oct 23, 2024 • edited Loading

Breaking Changes

github-actions bot commented Oct 23, 2024 • edited Loading

github-actions bot commented Oct 23, 2024 • edited Loading

rklaehn commented Oct 24, 2024

rklaehn commented Oct 24, 2024

dignifiedquire commented Oct 24, 2024

dignifiedquire commented Oct 24, 2024

rklaehn commented Oct 24, 2024

rklaehn commented Oct 24, 2024

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flub commented Oct 24, 2024

dignifiedquire commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dignifiedquire commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024 •

edited

Loading

github-actions bot commented Oct 23, 2024 •

edited

Loading