-
Notifications
You must be signed in to change notification settings - Fork 314
feat: Instead of duplicating the entire connection API, have a type parameter #3619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
on Connection that describes the connection state, and implement the fns alpn and remote_id differently depending on what the state is. The upside is that it should be easier to write code that works with both a 0rtt connection and a fully initialized connection without having to define a trait to abstract over both variants. The downside is that it is a bit fancy with the types. But then for the non 0rtt case you would never see the fanciness, so maybe it is OK?
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3619/docs/iroh/ Last updated: 2025-11-11T17:55:24Z |
…... structs Note: this requires using .connection() when you want to do something with the connection. You could avoid this with Deref, but it seems this way of using Deref is frowned upon in rust.
|
There are 3 ways to do the two Incoming/Outgoing... structs:
Maybe the last one is the way to go? |
they are now just type aliases for Connection<Incoming...> and Connection<Outgoing...> With this the usage is idential to before. No more .connection() needed for e.g. .open_bi().
|
Playing with this PR a bit, I think |
|
Also, it would be nice if we'd be able to downcast a E.g. that'd be useful to implement getting the remote id on a function that's generic over |
We could have a fourth state Maybe instead add methods to the trait that work in any case and return Option/Result? |
- export them and the trait - make the data types doc(hidden)
9cb5e4e to
a6e950c
Compare
@matheus23 so WDYT, should I just add endpoint_opt and alpn_opt that works for any state and be done with it, or do something more fancy to keep the API surface smaller? |
|
I was thinking of something like this: diff --git a/iroh/src/endpoint/connection.rs b/iroh/src/endpoint/connection.rs
index 6a4593adc..7f527708a 100644
--- a/iroh/src/endpoint/connection.rs
+++ b/iroh/src/endpoint/connection.rs
@@ -646,9 +646,20 @@ mod sealed {
}
/// Trait to track the state of a [`Connection`] at compile time.
-pub trait ConnectionState: sealed::Sealed {
+pub trait ConnectionState: sealed::Sealed + Sized {
/// State-specific data stored in the [`Connection`].
type Data: std::fmt::Debug + Clone;
+
+ /// Turns an arbitrary connection into an enum of possible typed connection states
+ fn into_state_enum(conn: Connection<Self>) -> StateEnum;
+}
+
+/// TODO docs
+#[derive(Debug)]
+pub enum StateEnum {
+ HandshakeCompleted(Connection<HandshakeCompleted>),
+ IncomingZeroRtt(Connection<IncomingZeroRtt>),
+ OutgoingZeroRtt(Connection<OutgoingZeroRtt>),
}
/// Marker type for a connection that has completed the handshake.
@@ -666,14 +677,26 @@ pub struct OutgoingZeroRtt;
impl sealed::Sealed for HandshakeCompleted {}
impl ConnectionState for HandshakeCompleted {
type Data = HandshakeCompletedData;
+
+ fn into_state_enum(conn: Connection<Self>) -> StateEnum {
+ StateEnum::HandshakeCompleted(conn)
+ }
}
impl sealed::Sealed for IncomingZeroRtt {}
impl ConnectionState for IncomingZeroRtt {
type Data = IncomingZeroRttData;
+
+ fn into_state_enum(conn: Connection<Self>) -> StateEnum {
+ StateEnum::IncomingZeroRtt(conn)
+ }
}
impl sealed::Sealed for OutgoingZeroRtt {}
impl ConnectionState for OutgoingZeroRtt {
type Data = OutgoingZeroRttData;
+
+ fn into_state_enum(conn: Connection<Self>) -> StateEnum {
+ StateEnum::OutgoingZeroRtt(conn)
+ }
}
#[allow(missing_docs)]
@@ -686,6 +709,11 @@ impl<T: ConnectionState> Connection<T> {
&self.inner
}
+ /// TODO docs
+ pub fn into_state_enum(self) -> StateEnum {
+ T::into_state_enum(self)
+ }
+
/// Initiates a new outgoing unidirectional stream.
///
/// Streams are cheap and instantaneous to open unless blocked by flow control. As aWhere But that'll cost us another exposed enum that we need to name and another exposed function on the Perhaps adding a |
I think the state enum will only be used very rarely, so my current thinking is that we don't do anything for now. The state enum is something people can define for themselves without much friction, much less annoying than having to define a trait. And even the try_remote_id and try_alpn are not strictly necessary. People can always do the match themselves before they go from concrete Connection to an abstract handler. How about we merge this now, frando and me hack it into the places where it is needed, which are already quite esoteric, and then we decide if we need more simplification. If we decide to do something about this, one option would also be to have a 4th state "generic". That would be the same number of additional public types, but would make usage nicer than an enum. E.g. for open_bi with the enum you would have to do a match, open_bi with the 4th state you could just use as normal. But again, I think there is not that much need to solve this right now. 1.0 means we can't modify, but we can still add syntax candy like this. |
Description
on Connection that describes the connection state, and implement the fns alpn and remote_id differently depending on what the state is.
The upside is that it should be easier to write code that works with both a 0rtt connection and a fully initialized connection without having to define a trait to abstract over both variants.
The downside is that it is a bit fancy with the types. But then for the non 0rtt case you would never see the fanciness, so maybe it is OK?
Todo:
Note:
if you have a handler that needs to work with both handshake completed connections and non handshake completed connections, you could now just do:
Breaking Changes
Notes & open questions
Change checklist
quic-rpciroh-gossipiroh-blobsdumbpipesendme