-
Notifications
You must be signed in to change notification settings - Fork 184
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
refactor(iroh-net): move all timeouts into one file #2641
Conversation
Co-locates all defaults that have to do with making connections or running netcheck.
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2641/docs/iroh/ Last updated: 2024-08-19T16:34:33Z |
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.
I'm kind of -1 on this. While it is nice and ordered right now I think it will be hard to maintain this. New timeouts will be added and not make it here because there's nothing naturally pointing here when you're in new code. const
items are now also treated differently whether they're a timeout or something else which is also pretty arbitrary.
I agree it is nice to see everything in one place and with docs. Perhaps that's something that should be possible with a bit of grep though. (rg -B 3 ': Duration = Duration::from'
is probably a decent start).
So I"m mainly worried this will result in a much messier code structure in the long term.
Well this is the first round, eventually these will be configurable, making this more sensible I believe |
@ramfox I don't think you are updating the DNS resolver configuration, or the quinn connection configuration yet |
@dignifiedquire we currently just use the defaults for quinn (10s) and the dns resolver (5s), so there are no |
Description
Co-locates all timeout defaults that have to do with making connections or running netcheck.
closes #2602
Notes & open questions
I nested some of the timeouts in another module (specifically the ones pertaining to the relay) but I could do this more for other sections, ie, the portmapper timeouts could be nested in a
portmapper
module.Change checklist