feat: Add Downloader::with_pool_options() constructor#213
feat: Add Downloader::with_pool_options() constructor#213rklaehn merged 3 commits inton0-computer:mainfrom
Conversation
Expose a way to create a Downloader with custom connection pool options (idle_timeout, connect_timeout, max_connections) instead of always using the hardcoded defaults. This allows callers to configure connection reuse behavior to reduce connection churn in long-running transfers.
Ugh. The path IDs start from 0 for each new connection. So connection churn should help with this. I'm really surprised you see a single connection churn through 2**32 paths. I don't have a lot of context but I suspect this might be the It would be great if you could file a separate issue for this and provide a bit more logs around it. Because I think that is part of the multipath teething problems. (I'm leaving considering the actual PR to someone more familiar with blobs, this doesn't mean the PR is wrong) |
|
Thank for the feedback @flub ! Great so I didn't know path IDs start from 0 for each new connection . So we shouldn't be exhausting all path IDs. Our config uses We will test it and tell you how it goes Regarding the changes in the PR: I could not get the old log with Being able to configure the pool's |
rklaehn
left a comment
There was a problem hiding this comment.
Looks pretty straightforward.
See the comments, but nothing major.
Also, we might want to just call it new_with_opts(...).
We got this pattern here and all over other crates where we have a "convenient" fn foo and then a "configurable" fn foo_with_opts. We might need more options later, and we don't want to have a giant list of new_with_x_and_y fns.
Signed-off-by: pefontana <fontana.pedro93@gmail.com>
|
@pefontana now we get a dead code warning. I guess the internal thing doesn't need a fn new. Otherwise good to go. Don't worry about the cargo deny - I will fix this in a separate PR. |
|
|
@pefontana FYI we just released a new iroh and iroh-blobs, and this made it in. |
Adds a
Downloader::with_pool_options()constructor that allows configuring the internalConnectionPooloptions.Currently
Downloader::new()hardcodesDefault::default()for pool options, which means theidle_timeoutis always 5 seconds. TheConnectionPoolandOptionstypes are already public, but there's no way to pass them through to theDownloader.We're from the Psyche/Nousnet team
In our use case, peers exchange gradient via iroh-blobs. With the 5s idle timeout, every connection gets dropped between transfers and a new one is created for the next round.
On a devnet run with 6 peers, over 8 hours we see:
The
MaxPathIdReachedhappens because QUIC path IDs are monotonically increasing and never reused, constant connection churn burns through them.Being able to set a longer idle timeout (e.g. 60s) would let the pool reuse connections across transfers, eliminating the churn entirely.
Here is how we plan to use it on NousNet
https://github.com/PsycheFoundation/nousnet/pull/600/changes
Feedback is really appreciated