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

Add AsyncResolverBuilder and option to overide a pre-created cache #1923

Closed
wants to merge 2 commits into from

Conversation

hottea773
Copy link

This PR adds the ability to share a DNS Cache between different AsyncResolvers. It also add an AsyncResolverBuilder to avoid adding yet more new_with_... methods to the AsyncResolver.

The rough reasoning behind it was that we've got a few different threads running all of which need to make DNS requests, but we didn't want to be making unnecessary requests if we've already done a previous lookup; hence a desire for multiple AsyncResolvers to shared their cache.

Since we did this work initially, I've thought that perhaps we could just have each thread use a clone of an initial AsyncResolver, which would mean that they shared the cache, since it's an Arc, so perhaps this change is unnecessary?

So I guess I have a question for reviewers:

Would there be any performance (or other) impact to using cloned AsyncResolver across multiple threads at the same time. Looking through the struct, I see that there are a few other items behind Arcs, such as https://github.com/bluejekyll/trust-dns/blob/main/crates/resolver/src/name_server/name_server_pool.rs#L39-L40 which I fear might cause some kind of issues, but don't really feel like I know enough to know.

If you don't think there would be any impact from that, then this PR is probably unnecessary (unless you disagree).

@djc
Copy link
Member

djc commented Apr 28, 2023

It is definitely expected to be cheap to clone an AsyncResolver and being able to use clones across threads.

@bluejekyll
Copy link
Member

A question I have, how would the shared cache operate differently than just shared instances of the AsyncResolver itself? It could be shared with separately configured resolvers, I'm not clear if that's necessarily a benefit or not?

@hottea773
Copy link
Author

Hey, thanks.
So we've decided to just use a cloned AsyncResolver for our use case, so I was coming back to close this PR. I did just see @bluejekyll's comment, and I guess sharing a cache between differently configured AsyncResolvers could be a benefit over an above what currently exists. (Although I'm sure it could be a footgun if used wrong.)

I'd appreciate your thoughts on whether you see any benefit in this PR, or whether I should just close it as unnecessary.

@bluejekyll
Copy link
Member

I don't really want to add complexity unless someone has a direct use case for it. It sounds like you do not have that requirement, but someone might have that in the future, and if so, then they could come back and resurrect this PR.

If you're ok with that, then let's just close this until someone has a need for the use case I mentioned.

@hottea773
Copy link
Author

Sounds sensible to me.

@hottea773 hottea773 closed this Jun 6, 2023
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.

None yet

3 participants