refactor(iroh): Allow to register custom protocols #2358

Frando · 2024-06-12T14:44:25Z

Description

feat(iroh-net): Allow to set the list of accepted ALPN protocols at runtime
refactor(iroh): Spawning a node can now be performed in two stages: First Builder::build() is called, which returns a Future that resolves to a new type ProtocolBuilder. The ProtocolBuilder is then spawned into the actual, running Node. If the intermediate step is not needed, Builder::spawn performs both in one call, therefore this change is not breaking.
feat(iroh): Allow to accept custom ALPN protocols in an Iroh node. Introduce a ProtocolHandler trait for accepting incoming connections and adds ProtocolBuilder::accept to register these handlers per ALPN.
refactor(iroh): Move towards more structured concurrency by spawning tasks into a JoinSet
refactor(iroh): Improve shutdown flow and perform more things concurently.

originally based on #2357 but now directly to main

Breaking Changes

iroh_net::endpoint::make_server_config takes Arc<quinn::TransportConfig> instead of Option<quinn::TransportConfig>. If you used the None case, replace with quinn::TransportConfig::default().

Notes & open questions

Change checklist

Self-review.
Documentation updates if relevant.
Tests if relevant.
All breaking changes documented.

iroh/src/node.rs

iroh-blobs/src/store/traits.rs

iroh-net/src/endpoint.rs

iroh/src/node/builder.rs

iroh/src/node.rs

rklaehn · 2024-06-19T08:19:50Z

iroh/src/node/builder.rs

+/// Note that the client returned from [`Self::client`] can only be used after spawning the node,
+/// until then all RPC calls will time out.
+#[derive(derive_more::Debug)]
+pub struct UnspawnedNode<D, E> {


Maybe call this CustomProtocolBuilder since you will use it only when you want to register custom protocols? Naming 🤷

Went with ProtocolBuilder for now. Open to changing further :)

iroh/src/node/protocol.rs

rklaehn

Added a bunch of comments. Overall looks great.

Frando · 2024-06-19T10:21:54Z

Addressed most review comments. And:

Renamed UnspawnedNode to ProtocolBuilder

Other public API namings we could bikeshed a bit:

trait Protocol: Fine as is? Or better ProtocolHandler?
ProtocolBuilder::accept(&self, alpn: &'static [u8], protocol: Arc<dyn Protocol>: Fine as is? Or better register_protocol?

I think I'm fine with the current namings, but also very open to changing if others find other names more intuitive.

rklaehn · 2024-06-19T10:36:33Z

trait Protocol: Fine as is? Or better ProtocolHandler?

I don't hate trait Protocol, but like ProtocolHandler a bit more. For me a protocol is a rust module containing a bunch of request and response types, usually in some enums, and some help text describing how to use them (e.g. serialize them with postcard), not something active. Or maybe AcceptHandler? After all the "active/outgoing" part of the protocol might live somewhere else.

ProtocolBuilder::accept(&self, alpn: &'static [u8], protocol: Arc: Fine as is? Or better register_protocol?

I think accept is fine. I mean, this is basically building a dispatcher for accept. As mentioned above, this part is only for the listen/accept part of the protocol, the active part will usually be created before using the iroh-net endpoint and then maybe end up in some tokio task.

rklaehn · 2024-06-19T10:51:46Z

Not sure if we want to address this in this PR or the next, but in Builder::build it now always calls register_iroh_protocols. So there is no way to opt out of them, just to add extra protocols.

I wasn't quite sure how it should look if you want to have a "naked" node. Also, it seems that we are not really ready to create a naked node. So maybe let's do it in the next PR that will allow disabling docs.

Frando · 2024-06-19T10:54:07Z

I renamed Protocol to ProtocolHandler. Agree with @rklaehn that this looks better.

Not sure if we want to address this in this PR or the next, but in Builder::build it now always calls register_iroh_protocols. So there is no way to opt out of them, just to add extra protocols.

Yup, let's defer this to a followup.

flub

I like this approach of structured concurrency, essentially the same as I did in iroh-relay. Also TIL about SharedAbortingJoinHandle. I'm a little unsure if there's any downsides to having many clones of the JoinHandle, but let's see. It makes it a lot easier to use.

flub · 2024-06-19T08:02:52Z

iroh-net/src/endpoint.rs

        })
    }

+    /// Set the list of accepted ALPN protocols.


Suggested change

/// Set the list of accepted ALPN protocols.

/// Sets the list of accepted ALPN protocols.

https://github.com/rust-lang/rfcs/blob/master/text/1574-more-api-documentation-conventions.md#appendix-a-full-conventions-text

flub · 2024-06-19T10:07:38Z

iroh/src/node/builder.rs

+        unspawned_node.spawn().await
+    }
+
+    /// Build a node without spawning it.


Suggested change

/// Build a node without spawning it.

/// Builds a node without spawning it.

flub · 2024-06-19T10:08:02Z

iroh/src/node/builder.rs

+
+    /// Build a node without spawning it.
+    ///
+    /// Returns an `UnspawnedNode`, on which custom protocols can be registered with


Suggested change

/// Returns an `UnspawnedNode`, on which custom protocols can be registered with

/// Returns an [`UnspawnedNode`], on which custom protocols can be registered with

flub · 2024-06-19T10:57:00Z

iroh/src/node/builder.rs

+}
+
+impl<D: iroh_blobs::store::Store, E: ServiceEndpoint<RpcService>> ProtocolBuilder<D, E> {
+    /// Register a protocol handler for incoming connections.


Suggested change

/// Register a protocol handler for incoming connections.

/// Registers a protocol handler for incoming connections.

flub · 2024-06-19T10:57:45Z

iroh/src/node/builder.rs

+        self
+    }
+
+    /// Return a client to control this node over an in-memory channel.


Suggested change

/// Return a client to control this node over an in-memory channel.

/// Returns a client to control this node over an in-memory channel.

flub · 2024-06-19T11:00:02Z

iroh/src/node/builder.rs

+        self.protocols.get_typed(alpn)
+    }
+
+    /// Register the core iroh protocols (blobs, gossip, docs).


Suggested change

/// Register the core iroh protocols (blobs, gossip, docs).

/// Registers the core iroh protocols (blobs, gossip, docs).

flub · 2024-06-19T11:02:27Z

iroh/src/node/builder.rs

+        self
+    }
+
+    /// Spawn the node and start accepting connections.


Suggested change

/// Spawn the node and start accepting connections.

/// Spawns the node and start accepting connections.

flub · 2024-06-19T11:05:06Z

iroh/src/node/protocol.rs

+
+impl ProtocolMap {
+    /// Returns the registered protocol handler for an ALPN as a concrete type.
+    pub fn get_typed<P: ProtocolHandler>(&self, alpn: &[u8]) -> Option<Arc<P>> {


Could we have the same visibility marker here as on the struct, so pub(super)? I find that a lot easier to read. Plus there's fewer surprises: if the super module decides to export this than suddenly these functions are really pub, and that might have been unintended.

flub · 2024-06-19T11:05:15Z

iroh/src/node/protocol.rs

+        self.0.get(alpn).cloned()
+    }
+
+    /// Insert a protocol handler.


Suggested change

/// Insert a protocol handler.

/// Inserts a protocol handler.

flub · 2024-06-19T11:05:30Z

iroh/src/node/protocol.rs

+        self.0.keys()
+    }
+
+    /// Shutdown all protocol handlers.


Suggested change

/// Shutdown all protocol handlers.

/// Shuts down all protocol handlers.

@flub

## Description Addresses review by @flub on #2358 (which is already merged). ## Breaking Changes  ## Notes & open questions  ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [ ] ~~Tests if relevant.~~ - [x] All breaking changes documented.

* feat(iroh-net): Allow to set the list of accepted ALPN protocols at runtime * refactor(iroh): Spawning a node can now be performed in two stages: First `Builder::build()` is called, which returns a Future that resolves to a new type `ProtocolBuilder`. The `ProtocolBuilder` is then spawned into the actual, running `Node`. If the intermediate step is not needed, `Builder::spawn` performs both in one call, therefore this change is not breaking. * feat(iroh): Allow to accept custom ALPN protocols in an Iroh node. Introduce a `ProtocolHandler` trait for accepting incoming connections and adds `ProtocolBuilder::accept` to register these handlers per ALPN. * refactor(iroh): Move towards more structured concurrency by spawning tasks into a `JoinSet` * refactor(iroh): Improve shutdown flow and perform more things concurently. originally based on n0-computer#2357 but now directly to main * `iroh_net::endpoint::make_server_config` takes `Arc<quinn::TransportConfig>` instead of `Option<quinn::TransportConfig>`. If you used the `None` case, replace with `quinn::TransportConfig::default()`.  - [x] Self-review. - [x] Documentation updates if relevant. - [ ] Tests if relevant. - [x] All breaking changes documented.

@flub

## Description Addresses review by @flub on n0-computer#2358 (which is already merged). ## Breaking Changes  ## Notes & open questions  ## Change checklist - [x] Self-review. - [x] Documentation updates if relevant. - [ ] ~~Tests if relevant.~~ - [x] All breaking changes documented.

Frando added 9 commits June 11, 2024 18:37

feat: custom protocols

79071f4

fixes and add example

aa1d78a

improve example

62dda48

improve example

2b14986

make builder send again

9261259

fix & clippy

85e40de

cleanups and PR review

ee043e5

improve code structure

db35136

fixup

c7517ce

dignifiedquire reviewed Jun 12, 2024

View reviewed changes

iroh/src/node.rs Outdated Show resolved Hide resolved

Frando force-pushed the optional-protocols branch from a439a5f to 9a46f6f Compare June 12, 2024 16:27

Frando added 2 commits June 12, 2024 18:32

rename back to accept

11a609f

refactor: use new protocols api and allow to disable docs

a369048

Frando force-pushed the optional-protocols branch from 9a46f6f to a369048 Compare June 12, 2024 16:35

Frando added 6 commits June 12, 2024 18:41

improvements

63fbcc0

simplify

13d3bf6

fixup

f56bb8c

fixup

97ddd64

use JoinSet

f97826e

improve shutdown

1f10806

dignifiedquire added this to the v0.19.0 milestone Jun 13, 2024

Frando added 2 commits June 13, 2024 16:41

fix shutdown

41bb962

fix doctest

5703d6e

Frando force-pushed the optional-protocols branch from 40f734f to 5703d6e Compare June 13, 2024 22:08

Frando added 4 commits June 14, 2024 00:40

cleanup

f4abba5

use OnceCell not Mutex

1d54b14

docs

1c20c7b

further cleanup

0da4375

Frando changed the title ~~refactor(iroh): Use new protocols API and make docs engine optional~~ refactor(iroh): Allow to register custom protocols and make docs engine optional Jun 13, 2024

Frando changed the base branch from custom-protocols-new to main June 13, 2024 23:13

Frando added 5 commits June 18, 2024 23:23

refactor: concurrent shutdown

766c8e5

fix: no need for empty alpn call in endpoint builder

d1c75e3

Merge remote-tracking branch 'origin/main' into optional-protocols

78f9e2e

fix: doctest

c1d0e5d

Merge branch 'main' into optional-protocols

88a77fa