Skip to content
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

Merged
merged 7 commits into from
Aug 19, 2024
Merged

refactor(iroh-net): move all timeouts into one file #2641

merged 7 commits into from
Aug 19, 2024

Conversation

ramfox
Copy link
Contributor

@ramfox ramfox commented Aug 19, 2024

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

  • Self-review.
  • Documentation updates following the style guide, if relevant.

Kasey Huizinga added 2 commits August 18, 2024 21:39
Co-locates all defaults that have to do with making connections or running netcheck.
@ramfox ramfox self-assigned this Aug 19, 2024
Copy link

github-actions bot commented Aug 19, 2024

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

@ramfox ramfox added this to the v0.23.0 milestone Aug 19, 2024
Copy link
Contributor

@flub flub left a 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.

iroh-net/src/defaults.rs Outdated Show resolved Hide resolved
@dignifiedquire
Copy link
Contributor

I'm kind of -1 on this. While it is nice and ordered right now I think it will be hard to maintain this.

Well this is the first round, eventually these will be configurable, making this more sensible I believe

@dignifiedquire
Copy link
Contributor

@ramfox I don't think you are updating the DNS resolver configuration, or the quinn connection configuration yet

@ramfox
Copy link
Contributor Author

ramfox commented Aug 19, 2024

@dignifiedquire we currently just use the defaults for quinn (10s) and the dns resolver (5s), so there are no consts to move around. The only places that we adjust the max_idle_timeout for quinn is in tests. And we have 2 consts for dns timeouts, one for netcheck and one for the client relay, but those are timeouts that wrap the dns resolver, so they can only shorten the timeout, not lengthen it.

@ramfox ramfox added this pull request to the merge queue Aug 19, 2024
Merged via the queue into main with commit bb808b4 Aug 19, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

refactor: move ALL timeouts into one file
4 participants