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

Use an Arc to share stub resolver #393

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

WhyNotHugo
Copy link
Contributor

This somewhat simplifies usage for call sites. They no longer need to use &&StubResolver for some APIs, and a future can be handed off without being tied to the lifetime of the resolver.

The documentation already mentioned that the StubResolver was behind an Arc so I did not need to updated it. StubResolver now implements clone, which is cheap since it merely clones the Arc with the inner data.

This change is backwards-incompatible, and necessitates a bump in minor for the next unstable release.

Fixes: #175

@WhyNotHugo
Copy link
Contributor Author

v2: formatting

@WhyNotHugo WhyNotHugo force-pushed the arc-stub-resolver branch 2 times, most recently from ea465e3 to 87ffe8b Compare September 16, 2024 17:54
@partim
Copy link
Member

partim commented Oct 2, 2024

Thank you for the PR!

Would it make sense to offer both, i.e., the current resolver and, say, a SharedResolver that keeps the resolver behind an arc? It would need its own query type, but I suppose that’s fine?

@WhyNotHugo
Copy link
Contributor Author

Would it make sense to offer both, i.e., the current resolver and, say, a SharedResolver that keeps the resolver behind an arc?

I don't see the value in it.

The difference in performance is meaningless. Each time that a DNS request is executed and parsed, the new implementation creates an Arc instead of a double-reference. Dereferencing the Arc should have no overhead.

The existing API is pretty un-ergonomic to use too, necessitating a double-reference to be used, and it always needs to explicitly outlive queries and their response.

This somewhat simplifies usage for call sites. They no longer need to
use `&&StubResolver` for some APIs, and a future can be handed off
without being tied to the lifetime of the resolver.

The documentation already mentioned that the StubResolver was behind an
Arc so I did not need to updated it. StubResolver now implements clone,
which is cheap since it merely clones the Arc with the inner data.

This change is backwards-incompatible, and necessitates a bump in minor
for the next unstable release.

Fixes: NLnetLabs#175
@WhyNotHugo
Copy link
Contributor Author

Rebased and resolved conflicts.

@partim partim added the breaking A PR that includes a breaking change. label Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A PR that includes a breaking change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make the stub resolver shareable.
2 participants