-
Notifications
You must be signed in to change notification settings - Fork 313
feat(multipath): allow to monitor all connections #3610
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: Frando/remote-info
Are you sure you want to change the base?
Conversation
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3610/docs/iroh/ Last updated: 2025-11-06T11:26:05Z |
34fb289 to
c377204
Compare
c377204 to
3424780
Compare
e277ff0 to
49c7ce3
Compare
4aa6f6d to
0b74cec
Compare
49c7ce3 to
aca0515
Compare
8fa7c37 to
1bbdbf1
Compare
aca0515 to
be70ea4
Compare
|
Firstly, there's nothing here that I think is problematic. I could live with this, and the ConnectionInfo stuff is also reasonable on its own. Though the exposed stats have some questions that I raised in the other PR. But that doesn't mean I'm not going to question things :)
Main downside of a hook is that a user can mess up the magicsock itself by misbehaving. OTOH a broadcast stream would pub a tokio type in our public API. Perhaps overall the callback is the right tradeoff, though I'd like to check if we already have any established patterns for this in the API and see what other's opinions are. /// Called for each new connection the endpoint accepts or initiates.
///
/// This is only called when a connection is fully established.
fn on_connection(&self, connection: ConnectionInfo);This immediately makes me wonder how soon folks will ask for something similar but earlier, on Would it be that bad to force anyone that wants to observe connections from a protocol they don't control to use the router? |
Oh, now I remember: it's outgoing connections that you can't track from the router. |
Description
Based on #3614, which adds
ConnectionInfo, a weak reference to a connection.This PR builds on that and adds a hook to the endpoint builder which allows implementors to observe all connections accepted or initiated by the endpoint.
The connection monitor is a new trait
ConnectionMonitorwith a single, synchronous methodon_connection(&self, connection: ConnectionInfo). This is called for each incoming or outgoing connection. It is only called once the connection has handshaked. The trait is also implemented forFn(ConnectionInfo)so you can also pass a closure to the endpoint builder function.Why a hook in the builder? We considered two other approaches:
With the hook, the question of how to deal with this is up to implementors. You get a synchronous callback that may not block, and can make your own tradeoffs: Send over an unbounded channel, send over a broadcast channel, insert into a RwLock'd data structre, ...
With the hook being on the builder and not the endpoint, this trade-off is quite explicit because it can only be implemented once. If you need to monitor connections from multiple places, you have to dispatch yourself and decide what tradeoffs are good for your app.
The latest commit in this branch uses n0-computer/quinn#153 to add a
closedmethod to theConnectionInfo. This allows to get theConnectionStatsat the time when the connection closes, without keeping the connection alive by awaiting the future. It is used in themonitor-connectionexample to show the final stats for connections once they are closed.Breaking Changes
Notes & open questions
Change checklist
quic-rpciroh-gossipiroh-blobsdumbpipesendme