Skip to content

fix: increase DNS timeout and address lookup stagger intervals#4008

Merged
Frando merged 4 commits intomainfrom
Frando/dns-timings
Mar 11, 2026
Merged

fix: increase DNS timeout and address lookup stagger intervals#4008
Frando merged 4 commits intomainfrom
Frando/dns-timings

Conversation

@Frando
Copy link
Copy Markdown
Member

@Frando Frando commented Mar 11, 2026

Description

  • Increases DNS_TIMEOUT from 1s to 3s
  • When looking up endpoint info via DnsAddressLookup, add more staggered calls up to 3s to not give up as quick as before
  • Don't ever cache negative responses in our DNS resolver
  • Slightly improve logging about DnsAddressLookup so we can debug more issues better.

Breaking Changes

Notes & open questions

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.
    • List all breaking changes in the above "Breaking Changes" section.
    • Open an issue or PR on any number0 repos that are affected by this breaking change. Give guidance on how the updates should be handled or do the actual updates themselves. The major ones are:

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 11, 2026

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/4008/docs/iroh/

Last updated: 2026-03-11T14:04:18Z

) -> Option<BoxStream<Result<AddressLookupItem, AddressLookupError>>> {
let resolver = self.dns_resolver.clone();
let origin_domain = self.origin_domain.clone();
let span = debug_span!("DnsAddressLookup", id=%endpoint_id.fmt_short(), %origin_domain);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The id logged is the one of the endpoint being looked up? Can we make that a bit clearer? Maybe lookup_id or query_id or so? I'm not entirely sure.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

/// Default timeout for DNS queries issued by [`DnsResolver`].
///
/// [`DnsResolver`]: crate::dns::DnsResolver
pub const DNS_TIMEOUT: Duration = Duration::from_secs(3);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, not sure I agree with this. Just to make the docs happy is not enough of a reason to make something pub. I would rather just hardcode the value in the docs.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I can revert. I thought it might still be useful, because downstream code might want to do calculations with this timeout. I.e. apply their own timeouts by adding something to this value. But can also revert, dunno?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert. It is unrelated to the thing we're fixing here and this is kind of a hotfix. So it's always better to keep the changes minimal even if we eventually decide to make this pub for some good reason.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted it.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 11, 2026

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 4d78045

@Frando Frando requested a review from flub March 11, 2026 14:01
@Frando Frando added this pull request to the merge queue Mar 11, 2026
Merged via the queue into main with commit 993b018 Mar 11, 2026
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants