Repository: seanmonstar/reqwest (language: rust)

Issue:
Re-enable trust-dns feature
The `trust-dns` feature was disabled in #617 as the dependency was not yet updated to `std::future`. We should re-enable once it is available.


Discussion / hints:
This was fixed here: https://github.com/bluejekyll/trust-dns/pull/849.

EDIT: So basically from version `0.18.0-alpha.1`.
We can re-enable once the version is out of alpha!
Some notes as I tried to re-enable this:

- `ResolveError` implements `Fail` instead of `std::error::Error`. I can get around this since it also implements `Into<io::Error>`, but it'd be better to not include `failure` in the dependency tree.
- `AsyncResolver` requires passing a `tokio::runtime::Handle` to its constructors, but we don't have one and have no way of materializing one.
> * `ResolveError` implements `Fail` instead of `std::error::Error`. I can get around this since it also implements `Into<io::Error>`, but it'd be better to not include `failure` in the dependency tree.

It still implements `Display`, wihch is what we are using as far as I can see:
https://github.com/seanmonstar/reqwest/blob/35c6ddd00b20243106e11ca9aa46abbc33d56bc9/src/dns.rs#L73
In any case, `trust-dns` pulls in `failure` anyway, so no loss there.
> * `AsyncResolver` requires passing a `tokio::runtime::Handle` to its constructors, but we don't have one and have no way of materializing one.

See https://github.com/bluejekyll/trust-dns/issues/977 and https://github.com/tokio-rs/tokio/pull/2040.

I would like to take a stab at this when the `Handle` is resolved, if thats alright.
@seanmonstar, we can remove all depdencies on Fail and failure. We'll lose backtrace, but that is coming in std.

might need a 0.19 release of trust-dns...
FYI: https://github.com/bluejekyll/trust-dns/pull/988

This drops `failure`, uses `thiserror` for some std error generation. Is there anything else? I’m planning to do a 0.19 release this weekend to get this out.
Ok, 0.19 is released.

Fix patch:
diff --git a/Cargo.toml b/Cargo.toml
index 5de905455..90b01ac44 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -45,7 +45,7 @@ brotli = ["async-compression", "async-compression/brotli"]
 
 json = ["serde_json"]
 
-#trust-dns = ["trust-dns-resolver"]
+trust-dns = ["trust-dns-resolver"]
 
 stream = []
 
@@ -113,7 +113,7 @@ async-compression = { version = "0.3.0", default-features = false, features = ["
 tokio-socks = { version = "0.2", optional = true }
 
 ## trust-dns
-#trust-dns-resolver = { version = "0.11", optional = true }
+trust-dns-resolver = { version = "0.19", optional = true }
 
 [target.'cfg(not(target_arch = "wasm32"))'.dev-dependencies]
 env_logger = "0.6"
diff --git a/src/connect.rs b/src/connect.rs
index a6557d7ab..f2cb647ec 100644
--- a/src/connect.rs
+++ b/src/connect.rs
@@ -19,8 +19,8 @@ use std::time::Duration;
 use std::mem::MaybeUninit;
 use pin_project_lite::pin_project;
 
-//#[cfg(feature = "trust-dns")]
-//use crate::dns::TrustDnsResolver;
+#[cfg(feature = "trust-dns")]
+use crate::dns::TrustDnsResolver;
 use crate::proxy::{Proxy, ProxyScheme};
 use crate::error::BoxError;
 #[cfg(feature = "default-tls")]
@@ -28,9 +28,9 @@ use self::native_tls_conn::NativeTlsConn;
 #[cfg(feature = "rustls-tls")]
 use self::rustls_tls_conn::RustlsTlsConn;
 
-//#[cfg(feature = "trust-dns")]
-//type HttpConnector = hyper::client::HttpConnector<TrustDnsResolver>;
-//#[cfg(not(feature = "trust-dns"))]
+#[cfg(feature = "trust-dns")]
+type HttpConnector = hyper::client::HttpConnector<TrustDnsResolver>;
+#[cfg(not(feature = "trust-dns"))]
 type HttpConnector = hyper::client::HttpConnector;
 
 #[derive(Clone)]
@@ -413,14 +413,14 @@ fn into_uri(scheme: Scheme, host: Authority) -> Uri {
         .expect("scheme and authority is valid Uri")
 }
 
-//#[cfg(feature = "trust-dns")]
-//fn http_connector() -> crate::Result<HttpConnector> {
-//    TrustDnsResolver::new()
-//        .map(HttpConnector::new_with_resolver)
-//        .map_err(crate::error::dns_system_conf)
-//}
+#[cfg(feature = "trust-dns")]
+fn http_connector() -> crate::Result<HttpConnector> {
+    TrustDnsResolver::new()
+        .map(HttpConnector::new_with_resolver)
+        .map_err(crate::error::builder)
+}
 
-//#[cfg(not(feature = "trust-dns"))]
+#[cfg(not(feature = "trust-dns"))]
 fn http_connector() -> crate::Result<HttpConnector> {
     Ok(HttpConnector::new())
 }
diff --git a/src/dns.rs b/src/dns.rs
index a76749dab..6df42138d 100644
--- a/src/dns.rs
+++ b/src/dns.rs
@@ -1,76 +1,91 @@
 use std::future::Future;
-use std::net::IpAddr;
-use std::sync::{Arc, Mutex, Once};
-use std::{io, vec};
+use std::pin::Pin;
+use std::sync::Arc;
+use std::task::{self, Poll};
+use std::io;
 
-use futures_util::future;
 use hyper::client::connect::dns as hyper_dns;
-use tokio;
-use trust_dns_resolver::{system_conf, AsyncResolver, BackgroundLookupIp};
+use hyper::service::Service;
+use tokio::sync::Mutex;
+use trust_dns_resolver::{
+    config::{ResolverConfig, ResolverOpts},
+    lookup_ip::LookupIpIntoIter,
+    system_conf, AsyncResolver, TokioConnection, TokioConnectionProvider,
+};
 
-// If instead the type were just `AsyncResolver`, it breaks the default recursion limit
-// for the compiler to determine if `reqwest::Client` is `Send`. This is because `AsyncResolver`
-// has **a lot** of internal generic types that pushes us over the limit.
-//
-// "Erasing" the internal resolver type saves us from this limit.
-type ErasedResolver = Box<dyn Fn(hyper_dns::Name) -> BackgroundLookupIp + Send + Sync>;
-type Background = Box<dyn Future<Item = (), Error = ()> + Send>;
+use crate::error::BoxError;
+
+type SharedResolver = Arc<AsyncResolver<TokioConnection, TokioConnectionProvider>>;
+
+lazy_static! {
+    static ref SYSTEM_CONF: io::Result<(ResolverConfig, ResolverOpts)> = system_conf::read_system_conf();
+}
 
 #[derive(Clone)]
 pub(crate) struct TrustDnsResolver {
-    inner: Arc<Inner>,
+    state: Arc<Mutex<State>>,
 }
 
-struct Inner {
-    background: Mutex<Option<Background>>,
-    once: Once,
-    resolver: ErasedResolver,
+enum State {
+    Init,
+    Ready(SharedResolver),
 }
 
 impl TrustDnsResolver {
     pub(crate) fn new() -> io::Result<Self> {
-        let (conf, opts) = system_conf::read_system_conf()?;
-        let (resolver, bg) = AsyncResolver::new(conf, opts);
-
-        let resolver: ErasedResolver = Box::new(move |name| resolver.lookup_ip(name.as_str()));
-        let background = Mutex::new(Some(Box::new(bg) as Background));
-        let once = Once::new();
+        SYSTEM_CONF.as_ref().map_err(|e| {
+            io::Error::new(e.kind(), format!("error reading DNS system conf: {}", e))
+        })?;
 
+        // At this stage, we might not have been called in the context of a
+        // Tokio Runtime, so we must delay the actual construction of the
+        // resolver.
         Ok(TrustDnsResolver {
-            inner: Arc::new(Inner {
-                background,
-                once,
-                resolver,
-            }),
+            state: Arc::new(Mutex::new(State::Init)),
         })
     }
 }
 
-impl hyper_dns::Resolve for TrustDnsResolver {
-    type Addrs = vec::IntoIter<IpAddr>;
-    type Future = Box<dyn Future<Output = Result<Self::Addrs, io::Error>> + Send>;
+impl Service<hyper_dns::Name> for TrustDnsResolver {
+    type Response = LookupIpIntoIter;
+    type Error = BoxError;
+    type Future = Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>> + Send>>;
+
+    fn poll_ready(&mut self, _: &mut task::Context<'_>) -> Poll<Result<(), Self::Error>> {
+        Poll::Ready(Ok(()))
+    }
+
+    fn call(&mut self, name: hyper_dns::Name) -> Self::Future {
+        let resolver = self.clone();
+        Box::pin(async move {
+            let mut lock = resolver.state.lock().await;
 
-    fn resolve(&self, name: hyper_dns::Name) -> Self::Future {
-        let inner = self.inner.clone();
-        Box::new(future::lazy(move || {
-            inner.once.call_once(|| {
-                // The `bg` (background) future needs to be spawned onto an executor,
-                // but a `reqwest::Client` may be constructed before an executor is ready.
-                // So, the `bg` future cannot be spawned *until* the executor starts to
-                // `poll` this future.
-                let bg = inner
-                    .background
-                    .lock()
-                    .expect("resolver background lock")
-                    .take()
-                    .expect("background only taken once");
+            let resolver = match &*lock {
+                State::Init => {
+                    let resolver = new_resolver(tokio::runtime::Handle::current()).await?;
+                    *lock = State::Ready(resolver.clone());
+                    resolver
+                },
+                State::Ready(resolver) => resolver.clone(),
+            };
 
-                tokio::spawn(bg);
-            });
+            // Don't keep lock once the resolver is constructed, otherwise
+            // only one lookup could be done at a time.
+            drop(lock);
 
-            (inner.resolver)(name)
-                .map(|lookup| lookup.iter().collect::<Vec<_>>().into_iter())
-                .map_err(|err| io::Error::new(io::ErrorKind::Other, err.to_string()))
-        }))
+            let lookup = resolver.lookup_ip(name.as_str()).await?;
+            Ok(lookup.into_iter())
+        })
     }
 }
+
+/// Takes a `Handle` argument as an indicator that it must be called from
+/// within the context of a Tokio runtime.
+async fn new_resolver(handle: tokio::runtime::Handle) -> Result<SharedResolver, BoxError> {
+    let (config, opts) = SYSTEM_CONF
+        .as_ref()
+        .expect("can't construct TrustDnsResolver if SYSTEM_CONF is error")
+        .clone();
+    let resolver = AsyncResolver::new(config, opts, handle).await?;
+    Ok(Arc::new(resolver))
+}
diff --git a/src/lib.rs b/src/lib.rs
index c26715784..99cc25079 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -177,6 +177,8 @@
 //! - **json**: Provides serialization and deserialization for JSON bodies.
 //! - **stream**: Adds support for `futures::Stream`.
 //! - **socks**: Provides SOCKS5 proxy support.
+//! - **trust-dns**: Enables a trust-dns async resolver instead of default
+//!   threadpool using `getaddrinfo`.
 //!
 //!
 //! [hyper]: http://hyper.rs
@@ -188,8 +190,6 @@
 //! [redirect]: crate::redirect
 //! [Proxy]: ./struct.Proxy.html
 //! [cargo-features]: https://doc.rust-lang.org/stable/cargo/reference/manifest.html#the-features-section
-////! - **trust-dns**: Enables a trust-dns async resolver instead of default
-////!   threadpool using `getaddrinfo`.
 
 macro_rules! if_wasm {
     ($($item:item)*) => {$(
@@ -295,8 +295,8 @@ if_hyper! {
     mod connect;
     #[cfg(feature = "cookies")]
     pub mod cookie;
-    //#[cfg(feature = "trust-dns")]
-    //mod dns;
+    #[cfg(feature = "trust-dns")]
+    mod dns;
     mod proxy;
     pub mod redirect;
     #[cfg(feature = "__tls")]


Test patch: