-
Notifications
You must be signed in to change notification settings - Fork 6
add a connection handler pool #36
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
So far no tests or anything, just to put it somewhere.
CC @b5, we talked about this. Something like this is useful for normal connections, but essential to deal with the situation where you want 0rtt connections for occasional short lived interactions and then flip to keeping the connection around if the interactions become more frequent. |
The original approach turns the connection pool into a combination of connection pool and task runner, and also is very unergonomic. So I ended up doing the following: There is a ConnectionRef which impls Deref but has an additional field that tracks lifetime of the ConnectionRef. You must keep the ConnectionRef alive while you are using the connection in any way, but there is no way around this short of completely changing quinn. When no more ConnectionRefs are used, the connection goes into idle state. After an idle timeout, it gets closed. If there is a demand for more connections before idle connections are closed, one of the idle connections (the oldest) is reclaimed. If there are no idle connections, and you are at capacity, you get an error. There is currently no way to get notified when connection slots are available. We could change this by making some statistics (e.g. total number of connections and number of idle connections) available via a n0_watcher::Watchable. |
we don't expect connect to panic, so we don't need to isolate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know that this connection pool measurably improves the situation compared to using 0-RTT connections?
In my mind it seems like keeping open a connection shouldn't really improve the situation for users much.
- If a connection is closed and re-established in iroh's TRUST_UDP_ADDR_DURATION (6.5s) then re-opening a connection shouldn't require us to re-hole-punch.
- If re-establishment is done in 0-RTT, then opening another connection shouldn't be that much more involved compared to sending some data on a stream. (I guess there's some overhead to establish the forward secrecy again, i.e. running ECDH, but otherwise?)
- Doesn't the user usually batch operations anyways, e.g. in iroh-blobs? Requiring that the user keeps a hold of some handle (which e.g. in turn owns a
Connection
) isn't too much to ask IMO. In that case, they wouldn't need a connection pool (or rather, they might need some other kind of pool, e.g. a "pool of iroh-blobs Batches" for example). Perhaps you have some personal experience that goes contrary to this. - How does this handle the accepting side going away during a connection without a graceful close (e.g. with a hard shutdown without calling
Endpoint::close().await
)? In my experience this caused the connection to become idle, and if we have a connection pool, the situation might get worse, because now every.connect
gives you back the defunct connection for 30s, causing delays you wouldn't get if you re-establish connections "normally".
FWIW I'd also like to understand if there's a way to distinguish terminology for a bunch of different "connection pools". I don't know how to call these different features:
- A connection pool that tries to keep only a single outgoing connection per NodeId (I think that's this impl)
- A connection pool that tries to keep around outgoing connections for just a little longer in case they're needed again immediately after (also this impl)
- A connection pool that tries to de-duplicate between incoming and outgoing connections, aiming towards having one connection open at a time (this is implemented in iroh-gossip)
- A connection pool that makes sure to re-establish connections with backoff as long as a user "wants to stay connected to " (this might be called a ConnectionManager instead, and I kinda want this for swarm CRDT use-cases, but also gossip)
Minor nit: Would be nice if you can edit the top-level README.md
with a link to the new example (I forgot to do that for iroh-automerge-repo as well, so I might get to it after your PR if you don't get to it before me).
This is a compromise that tries to provide a connection pool even though iroh/quinn design does not make this easy. Connections are Clone, but they want to be owned by the pool. So you get a chance to work with a connection but are not supposed to clone them out.
Questions:
Does the thing in the main actor have to be a JoinSet, or could it also just be a FuturesUnordered?
Do we need a way to watch currently active and idle connections?