[libp2p-dns] Use trust-dns-resolver (with either async-std or tokio). Remove thread pool. #1927
Conversation
Use the `trust-dns-resolver` library for DNS resolution, thereby removing current use of the thread pool. Since `trust-dns-resolver` and related crates already provide support for both `async-std` and `tokio`, we make use of that here in our own feature flags. Since `trust-dns-resolver` provides many useful configuration options and error detail, central types of `trust-dns-resolver` like `ResolverConfig`, `ResolverOpts` and `ResolveError` are re-exposed in the API of `libp2p-dns`. Full encapsulation does not seem preferable in this case.
There was a problem hiding this comment.
Code changes look good to me.
I don't hold a strong opinion on OS resolver vs. library resolver. In general I would consider DNS resolution a responsibility of the OS, at the same time https://github.com/bluejekyll/trust-dns seems to be well established and thus worth betting on.
| @@ -264,35 +261,27 @@ pub use self::simple::SimpleProtocol; | |||
| pub use self::swarm::Swarm; | |||
| pub use self::transport_ext::TransportExt; | |||
|
|
|||
| /// Builds a `Transport` that supports the most commonly-used protocols that libp2p supports. | |||
| /// Builds a `Transport` based on TCP/IP that supports the most commonly-used features of libp2p: | |||
There was a problem hiding this comment.
I am very much in favor of this cleanup.
There was a problem hiding this comment.
Changes look good to me.
I would suggest waiting for https://github.com/bluejekyll/trust-dns/pull/1354 to be released before merging here. What do you think?
|
Yes, of course. I am anyway already working on the follow-up PR for |
|
Unfortunately still pending a new release of |
@romanb are you aware of a timeline for this release to happen? I am happy to reach out to the authors myself. It is not urgent, though it would be nice to build on top of the general changes introduced via #1931. In addition it would be a bummer for this pull request and current |
No, but based on the prior release history of the crate I was indeed hoping for a new release some time in February. It would be nice if you could reach out. |
|
|
This PR changes
libp2p-dnsto use thetrust-dns-resolverlibrary for DNS resolution, thereby removing the current use of the thread pool, i.e. closing #1235. This also serves as a first step for #1920, which will build on this work.Since
trust-dns-resolverand related crates already provide decent support for bothasync-stdandtokio, we make use of that here inlibp2p-dns, offering analogousasync-stdandtokiofeatures. Just like withlibp2p-tcp, theasync-stdvariant is enabled by default. This PR is only pending a new release that includes https://github.com/bluejekyll/trust-dns/pull/1354, which is why it is still pointing to a git dependency. Also just like withlibp2p-tcp, thelibp2p-dnstests are exercised for both runtimes.Since
trust-dns-resolverprovides many useful configuration options and error details, central types oftrust-dns-resolverlikeResolverConfig,ResolverOptsandResolveErrorare re-exposed in the API oflibp2p-dns. Full encapsulationdoes not seem preferable in this case. We also re-export the (currently tokio-only) features for DNS-over-TLS and DNS-over-HTTPS (via rustls).
I also consolidated the top-level utility functions of the
libp2pcrate: There is now justfn development_transport()(available with default features) andfn tokio_development_transport()(available when the corresponding tokio features are enabled). I think that is sufficient and I removed the minor variation that also includedpnetsupport into the development transport.