From 319e8aee1571d8d3639b3259e7a1edb964e6a26c Mon Sep 17 00:00:00 2001 From: Sean McArthur Date: Wed, 4 Dec 2019 12:49:44 -0800 Subject: [PATCH] feat(client): remove `Destination` for `http::Uri` in connectors BREAKING CHANGE: All usage of `hyper::client::connect::Destination` should be replaced with `http::Uri`. --- benches/connect.rs | 5 +- src/client/connect/dns.rs | 5 +- src/client/connect/http.rs | 52 ++--- src/client/connect/mod.rs | 391 ++----------------------------------- src/client/mod.rs | 6 +- tests/client.rs | 11 +- 6 files changed, 40 insertions(+), 430 deletions(-) diff --git a/benches/connect.rs b/benches/connect.rs index e249af9e1b..b9d13198f5 100644 --- a/benches/connect.rs +++ b/benches/connect.rs @@ -5,7 +5,7 @@ extern crate test; use std::net::SocketAddr; use tokio::net::TcpListener; -use hyper::client::connect::{Destination, HttpConnector}; +use hyper::client::connect::{HttpConnector}; use hyper::service::Service; use http::Uri; @@ -19,8 +19,7 @@ fn http_connector(b: &mut test::Bencher) { .expect("rt build"); let mut listener = rt.block_on(TcpListener::bind(&SocketAddr::from(([127, 0, 0, 1], 0)))).expect("bind"); let addr = listener.local_addr().expect("local_addr"); - let uri: Uri = format!("http://{}/", addr).parse().expect("uri parse"); - let dst = Destination::try_from_uri(uri).expect("destination"); + let dst: Uri = format!("http://{}/", addr).parse().expect("uri parse"); let mut connector = HttpConnector::new(); rt.spawn(async move { diff --git a/src/client/connect/dns.rs b/src/client/connect/dns.rs index 68f8771460..3b8c44eacf 100644 --- a/src/client/connect/dns.rs +++ b/src/client/connect/dns.rs @@ -391,11 +391,10 @@ mod tests { #[test] fn ip_addrs_try_parse_v6() { - let uri = ::http::Uri::from_static("http://[::1]:8080/"); - let dst = super::super::Destination { uri }; + let dst = ::http::Uri::from_static("http://[::1]:8080/"); let mut addrs = - IpAddrs::try_parse(dst.host(), dst.port().expect("port")).expect("try_parse"); + IpAddrs::try_parse(dst.host().expect("host"), dst.port_u16().expect("port")).expect("try_parse"); let expected = "[::1]:8080".parse::().expect("expected"); diff --git a/src/client/connect/http.rs b/src/client/connect/http.rs index 1ae73ff647..9ab38cd7cf 100644 --- a/src/client/connect/http.rs +++ b/src/client/connect/http.rs @@ -17,7 +17,7 @@ use tokio::net::TcpStream; use tokio::time::Delay; use super::dns::{self, resolve, GaiResolver, Resolve}; -use super::{Connected, Destination}; +use super::{Connected}; //#[cfg(feature = "runtime")] use super::dns::TokioThreadpoolGaiResolver; @@ -229,7 +229,7 @@ impl fmt::Debug for HttpConnector { } } -impl tower_service::Service for HttpConnector +impl tower_service::Service for HttpConnector where R: Resolve + Clone + Send + Sync + 'static, R::Future: Send, @@ -243,7 +243,7 @@ where Poll::Ready(Ok(())) } - fn call(&mut self, dst: Destination) -> Self::Future { + fn call(&mut self, dst: Uri) -> Self::Future { let mut self_ = self.clone(); HttpConnecting { fut: Box::pin(async move { self_.call_async(dst).await }), @@ -258,30 +258,30 @@ where { async fn call_async( &mut self, - dst: Destination, + dst: Uri, ) -> Result<(TcpStream, Connected), ConnectError> { trace!( - "Http::connect; scheme={}, host={}, port={:?}", + "Http::connect; scheme={:?}, host={:?}, port={:?}", dst.scheme(), dst.host(), dst.port(), ); if self.config.enforce_http { - if dst.uri.scheme() != Some(&Scheme::HTTP) { + if dst.scheme() != Some(&Scheme::HTTP) { return Err(ConnectError { msg: INVALID_NOT_HTTP.into(), cause: None, }); } - } else if dst.uri.scheme().is_none() { + } else if dst.scheme().is_none() { return Err(ConnectError { msg: INVALID_MISSING_SCHEME.into(), cause: None, }); } - let host = match dst.uri.host() { + let host = match dst.host() { Some(s) => s, None => { return Err(ConnectError { @@ -290,9 +290,9 @@ where }) } }; - let port = match dst.uri.port() { + let port = match dst.port() { Some(port) => port.as_u16(), - None => if dst.uri.scheme() == Some(&Scheme::HTTPS) { 443 } else { 80 }, + None => if dst.scheme() == Some(&Scheme::HTTPS) { 443 } else { 80 }, }; let config = &self.config; @@ -351,26 +351,6 @@ where } } -impl tower_service::Service for HttpConnector -where - R: Resolve + Clone + Send + Sync + 'static, - R::Future: Send, -{ - type Response = TcpStream; - type Error = ConnectError; - type Future = - Pin> + Send + 'static>>; - - fn poll_ready(&mut self, cx: &mut task::Context<'_>) -> Poll> { - tower_service::Service::::poll_ready(self, cx) - } - - fn call(&mut self, uri: Uri) -> Self::Future { - let mut self_ = self.clone(); - Box::pin(async move { self_.call_async(Destination { uri }).await.map(|(s, _)| s) }) - } -} - impl HttpInfo { /// Get the remote address of the transport used. pub fn remote_addr(&self) -> SocketAddr { @@ -661,12 +641,14 @@ impl ConnectingTcp { mod tests { use std::io; + use ::http::Uri; + use super::super::sealed::Connect; - use super::{Connected, Destination, HttpConnector}; + use super::{Connected, HttpConnector}; async fn connect( connector: C, - dst: Destination, + dst: Uri, ) -> Result<(C::Transport, Connected), C::Error> where C: Connect, @@ -676,8 +658,7 @@ mod tests { #[tokio::test] async fn test_errors_enforce_http() { - let uri = "https://example.domain/foo/bar?baz".parse().unwrap(); - let dst = Destination { uri }; + let dst = "https://example.domain/foo/bar?baz".parse().unwrap(); let connector = HttpConnector::new(); let err = connect(connector, dst).await.unwrap_err(); @@ -686,8 +667,7 @@ mod tests { #[tokio::test] async fn test_errors_missing_scheme() { - let uri = "example.domain".parse().unwrap(); - let dst = Destination { uri }; + let dst = "example.domain".parse().unwrap(); let mut connector = HttpConnector::new(); connector.enforce_http(false); diff --git a/src/client/connect/mod.rs b/src/client/connect/mod.rs index 5c9aa85645..b6af79235e 100644 --- a/src/client/connect/mod.rs +++ b/src/client/connect/mod.rs @@ -5,24 +5,14 @@ //! - A default [`HttpConnector`](HttpConnector) that does DNS resolution and //! establishes connections over TCP. //! - Types to build custom connectors. -use std::convert::TryFrom; -use std::{fmt, mem}; +use std::fmt; -use bytes::{BufMut, Bytes, BytesMut}; -use ::http::{uri, Response, Uri}; +use ::http::{Response}; #[cfg(feature = "tcp")] pub mod dns; #[cfg(feature = "tcp")] mod http; #[cfg(feature = "tcp")] pub use self::http::{HttpConnector, HttpInfo}; -/// A set of properties to describe where and how to try to connect. -/// -/// This type is passed an argument to connectors. -#[derive(Clone, Debug)] -pub struct Destination { - pub(super) uri: Uri, -} - /// Extra information about the connected transport. /// /// This can be used to inform recipients about things like if ALPN @@ -42,205 +32,6 @@ pub(super) enum Alpn { None, } -impl Destination { - /// Try to convert a `Uri` into a `Destination` - /// - /// # Error - /// - /// Returns an error if the uri contains no authority or - /// no scheme. - pub fn try_from_uri(uri: Uri) -> crate::Result { - uri.authority().ok_or(crate::error::Parse::Uri)?; - uri.scheme().ok_or(crate::error::Parse::Uri)?; - Ok(Destination { uri }) - } - - /// Get the protocol scheme. - #[inline] - pub fn scheme(&self) -> &str { - self.uri - .scheme_str() - .unwrap_or("") - } - - /// Get the hostname. - #[inline] - pub fn host(&self) -> &str { - self.uri - .host() - .unwrap_or("") - } - - /// Get the port, if specified. - #[inline] - pub fn port(&self) -> Option { - self.uri.port_u16() - } - - /// Update the scheme of this destination. - /// - /// # Example - /// - /// ```rust - /// # use hyper::client::connect::Destination; - /// # fn with_dst(mut dst: Destination) { - /// // let mut dst = some_destination... - /// // Change from "http://"... - /// assert_eq!(dst.scheme(), "http"); - /// - /// // to "ws://"... - /// dst.set_scheme("ws"); - /// assert_eq!(dst.scheme(), "ws"); - /// # } - /// ``` - /// - /// # Error - /// - /// Returns an error if the string is not a valid scheme. - pub fn set_scheme(&mut self, scheme: &str) -> crate::Result<()> { - let scheme = scheme.parse().map_err(crate::error::Parse::from)?; - self.update_uri(move |parts| { - parts.scheme = Some(scheme); - }) - } - - /// Update the host of this destination. - /// - /// # Example - /// - /// ```rust - /// # use hyper::client::connect::Destination; - /// # fn with_dst(mut dst: Destination) { - /// // let mut dst = some_destination... - /// // Change from "hyper.rs"... - /// assert_eq!(dst.host(), "hyper.rs"); - /// - /// // to "some.proxy"... - /// dst.set_host("some.proxy"); - /// assert_eq!(dst.host(), "some.proxy"); - /// # } - /// ``` - /// - /// # Error - /// - /// Returns an error if the string is not a valid hostname. - pub fn set_host(&mut self, host: &str) -> crate::Result<()> { - // Prevent any userinfo setting, it's bad! - if host.contains('@') { - return Err(crate::error::Parse::Uri.into()); - } - let auth = if let Some(port) = self.port() { - let bytes = Bytes::from(format!("{}:{}", host, port)); - uri::Authority::from_maybe_shared(bytes) - .map_err(crate::error::Parse::from)? - } else { - let auth = host.parse::().map_err(crate::error::Parse::from)?; - if auth.port().is_some() { // std::uri::Authority::Uri - return Err(crate::error::Parse::Uri.into()); - } - auth - }; - self.update_uri(move |parts| { - parts.authority = Some(auth); - }) - } - - /// Update the port of this destination. - /// - /// # Example - /// - /// ```rust - /// # use hyper::client::connect::Destination; - /// # fn with_dst(mut dst: Destination) { - /// // let mut dst = some_destination... - /// // Change from "None"... - /// assert_eq!(dst.port(), None); - /// - /// // to "4321"... - /// dst.set_port(4321); - /// assert_eq!(dst.port(), Some(4321)); - /// - /// // Or remove the port... - /// dst.set_port(None); - /// assert_eq!(dst.port(), None); - /// # } - /// ``` - pub fn set_port

(&mut self, port: P) - where - P: Into>, - { - self.set_port_opt(port.into()); - } - - fn set_port_opt(&mut self, port: Option) { - use std::fmt::Write; - - let auth = if let Some(port) = port { - let host = self.host(); - // Need space to copy the hostname, plus ':', - // plus max 5 port digits... - let cap = host.len() + 1 + 5; - let mut buf = BytesMut::with_capacity(cap); - buf.put_slice(host.as_bytes()); - buf.put_u8(b':'); - write!(buf, "{}", port) - .expect("should have space for 5 digits"); - - uri::Authority::from_maybe_shared(buf.freeze()) - .expect("valid host + :port should be valid authority") - } else { - self.host().parse() - .expect("valid host without port should be valid authority") - }; - - self.update_uri(move |parts| { - parts.authority = Some(auth); - }) - .expect("valid uri should be valid with port"); - } - - fn update_uri(&mut self, f: F) -> crate::Result<()> - where - F: FnOnce(&mut uri::Parts) - { - // Need to store a default Uri while we modify the current one... - let old_uri = mem::replace(&mut self.uri, Uri::default()); - // However, mutate a clone, so we can revert if there's an error... - let mut parts: uri::Parts = old_uri.clone().into(); - - f(&mut parts); - - match Uri::from_parts(parts) { - Ok(uri) => { - self.uri = uri; - Ok(()) - }, - Err(err) => { - self.uri = old_uri; - Err(crate::error::Parse::from(err).into()) - }, - } - } - - /* - /// Returns whether this connection must negotiate HTTP/2 via ALPN. - pub fn must_h2(&self) -> bool { - match self.alpn { - Alpn::Http1 => false, - Alpn::H2 => true, - } - } - */ -} - -impl TryFrom for Destination { - type Error = crate::error::Error; - - fn try_from(uri: Uri) -> Result { - Destination::try_from_uri(uri) - } -} - impl Connected { /// Create new `Connected` type with empty metadata. pub fn new() -> Connected { @@ -372,22 +163,22 @@ where pub(super) mod sealed { use std::error::Error as StdError; + use ::http::Uri; use tokio::io::{AsyncRead, AsyncWrite}; use crate::common::{Future, Unpin}; - use super::{Connected, Destination}; + use super::{Connected}; /// Connect to a destination, returning an IO transport. /// - /// A connector receives a [`Destination`](Destination) describing how a - /// connection should be estabilished, and returns a `Future` of the + /// A connector receives a [`Uri`](::http::Uri) and returns a `Future` of the /// ready connection. /// /// # Trait Alias /// /// This is really just an *alias* for the `tower::Service` trait, with /// additional bounds set for convenience *inside* hyper. You don't actually - /// implement this trait, but `tower::Service` instead. + /// implement this trait, but `tower::Service` instead. // The `Sized` bound is to prevent creating `dyn Connect`, since they cannot // fit the `Connect` bounds because of the blanket impl for `Service`. pub trait Connect: Sealed + Sized { @@ -398,27 +189,27 @@ pub(super) mod sealed { /// A Future that will resolve to the connected Transport. type Future: Future>; #[doc(hidden)] - fn connect(self, internal_only: Internal, dst: Destination) -> Self::Future; + fn connect(self, internal_only: Internal, dst: Uri) -> Self::Future; } impl Connect for S where - S: tower_service::Service + Send, + S: tower_service::Service + Send, S::Error: Into>, S::Future: Unpin + Send, T: AsyncRead + AsyncWrite + Unpin + Send + 'static, { type Transport = T; type Error = S::Error; - type Future = crate::service::Oneshot; - fn connect(self, _: Internal, dst: Destination) -> Self::Future { + type Future = crate::service::Oneshot; + fn connect(self, _: Internal, dst: Uri) -> Self::Future { crate::service::oneshot(self, dst) } } impl Sealed for S where - S: tower_service::Service + Send, + S: tower_service::Service + Send, S::Error: Into>, S::Future: Unpin + Send, T: AsyncRead + AsyncWrite + Unpin + Send + 'static, @@ -431,165 +222,7 @@ pub(super) mod sealed { #[cfg(test)] mod tests { - use super::{Connected, Destination, TryFrom}; - - #[test] - fn test_destination_set_scheme() { - let mut dst = Destination { - uri: "http://hyper.rs".parse().expect("initial parse"), - }; - - assert_eq!(dst.scheme(), "http"); - assert_eq!(dst.host(), "hyper.rs"); - - dst.set_scheme("https").expect("set https"); - assert_eq!(dst.scheme(), "https"); - assert_eq!(dst.host(), "hyper.rs"); - - dst.set_scheme("").unwrap_err(); - assert_eq!(dst.scheme(), "https", "error doesn't modify dst"); - assert_eq!(dst.host(), "hyper.rs", "error doesn't modify dst"); - } - - #[test] - fn test_destination_set_host() { - let mut dst = Destination { - uri: "http://hyper.rs".parse().expect("initial parse"), - }; - - assert_eq!(dst.scheme(), "http"); - assert_eq!(dst.host(), "hyper.rs"); - assert_eq!(dst.port(), None); - - dst.set_host("seanmonstar.com").expect("set https"); - assert_eq!(dst.scheme(), "http"); - assert_eq!(dst.host(), "seanmonstar.com"); - assert_eq!(dst.port(), None); - - dst.set_host("/im-not a host! >:)").unwrap_err(); - assert_eq!(dst.scheme(), "http", "error doesn't modify dst"); - assert_eq!(dst.host(), "seanmonstar.com", "error doesn't modify dst"); - assert_eq!(dst.port(), None, "error doesn't modify dst"); - - // Check port isn't snuck into `set_host`. - dst.set_host("seanmonstar.com:3030").expect_err("set_host sneaky port"); - assert_eq!(dst.scheme(), "http", "error doesn't modify dst"); - assert_eq!(dst.host(), "seanmonstar.com", "error doesn't modify dst"); - assert_eq!(dst.port(), None, "error doesn't modify dst"); - - // Check userinfo isn't snuck into `set_host`. - dst.set_host("sean@nope").expect_err("set_host sneaky userinfo"); - assert_eq!(dst.scheme(), "http", "error doesn't modify dst"); - assert_eq!(dst.host(), "seanmonstar.com", "error doesn't modify dst"); - assert_eq!(dst.port(), None, "error doesn't modify dst"); - - // Allow IPv6 hosts - dst.set_host("[::1]").expect("set_host with IPv6"); - assert_eq!(dst.host(), "[::1]"); - assert_eq!(dst.port(), None, "IPv6 didn't affect port"); - - // However, IPv6 with a port is rejected. - dst.set_host("[::2]:1337").expect_err("set_host with IPv6 and sneaky port"); - assert_eq!(dst.host(), "[::1]"); - assert_eq!(dst.port(), None); - - // ----------------- - - // Also test that an exist port is set correctly. - let mut dst = Destination { - uri: "http://hyper.rs:8080".parse().expect("initial parse 2"), - }; - - assert_eq!(dst.scheme(), "http"); - assert_eq!(dst.host(), "hyper.rs"); - assert_eq!(dst.port(), Some(8080)); - - dst.set_host("seanmonstar.com").expect("set host"); - assert_eq!(dst.scheme(), "http"); - assert_eq!(dst.host(), "seanmonstar.com"); - assert_eq!(dst.port(), Some(8080)); - - dst.set_host("/im-not a host! >:)").unwrap_err(); - assert_eq!(dst.scheme(), "http", "error doesn't modify dst"); - assert_eq!(dst.host(), "seanmonstar.com", "error doesn't modify dst"); - assert_eq!(dst.port(), Some(8080), "error doesn't modify dst"); - - // Check port isn't snuck into `set_host`. - dst.set_host("seanmonstar.com:3030").expect_err("set_host sneaky port"); - assert_eq!(dst.scheme(), "http", "error doesn't modify dst"); - assert_eq!(dst.host(), "seanmonstar.com", "error doesn't modify dst"); - assert_eq!(dst.port(), Some(8080), "error doesn't modify dst"); - - // Check userinfo isn't snuck into `set_host`. - dst.set_host("sean@nope").expect_err("set_host sneaky userinfo"); - assert_eq!(dst.scheme(), "http", "error doesn't modify dst"); - assert_eq!(dst.host(), "seanmonstar.com", "error doesn't modify dst"); - assert_eq!(dst.port(), Some(8080), "error doesn't modify dst"); - - // Allow IPv6 hosts - dst.set_host("[::1]").expect("set_host with IPv6"); - assert_eq!(dst.host(), "[::1]"); - assert_eq!(dst.port(), Some(8080), "IPv6 didn't affect port"); - - // However, IPv6 with a port is rejected. - dst.set_host("[::2]:1337").expect_err("set_host with IPv6 and sneaky port"); - assert_eq!(dst.host(), "[::1]"); - assert_eq!(dst.port(), Some(8080)); - } - - #[test] - fn test_destination_set_port() { - let mut dst = Destination { - uri: "http://hyper.rs".parse().expect("initial parse"), - }; - - assert_eq!(dst.scheme(), "http"); - assert_eq!(dst.host(), "hyper.rs"); - assert_eq!(dst.port(), None); - - dst.set_port(None); - assert_eq!(dst.scheme(), "http"); - assert_eq!(dst.host(), "hyper.rs"); - assert_eq!(dst.port(), None); - - dst.set_port(8080); - assert_eq!(dst.scheme(), "http"); - assert_eq!(dst.host(), "hyper.rs"); - assert_eq!(dst.port(), Some(8080)); - - // Also test that an exist port is set correctly. - let mut dst = Destination { - uri: "http://hyper.rs:8080".parse().expect("initial parse 2"), - }; - - assert_eq!(dst.scheme(), "http"); - assert_eq!(dst.host(), "hyper.rs"); - assert_eq!(dst.port(), Some(8080)); - - dst.set_port(3030); - assert_eq!(dst.scheme(), "http"); - assert_eq!(dst.host(), "hyper.rs"); - assert_eq!(dst.port(), Some(3030)); - - dst.set_port(None); - assert_eq!(dst.scheme(), "http"); - assert_eq!(dst.host(), "hyper.rs"); - assert_eq!(dst.port(), None); - } - - #[test] - fn test_try_from_destination() { - let uri: http::Uri = "http://hyper.rs".parse().expect("initial parse"); - let result = Destination::try_from(uri); - assert_eq!(result.is_ok(), true); - } - - #[test] - fn test_try_from_no_scheme() { - let uri: http::Uri = "hyper.rs".parse().expect("initial parse error"); - let result = Destination::try_from(uri); - assert_eq!(result.is_err(), true); - } + use super::{Connected}; #[derive(Clone, Debug, PartialEq)] struct Ex1(usize); diff --git a/src/client/mod.rs b/src/client/mod.rs index 81f9f52dbd..c980d1b449 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -71,7 +71,7 @@ use http::uri::Scheme; use crate::body::{Body, Payload}; use crate::common::{lazy as hyper_lazy, BoxSendFuture, Executor, Lazy, Future, Pin, Poll, task}; -use self::connect::{Alpn, sealed::Connect, Connected, Destination}; +use self::connect::{Alpn, sealed::Connect, Connected}; use self::pool::{Key as PoolKey, Pool, Poolable, Pooled, Reservation}; #[cfg(feature = "tcp")] pub use self::connect::HttpConnector; @@ -462,9 +462,7 @@ where C: Connect + Clone + Send + Sync + 'static, let ver = self.config.ver; let is_ver_h2 = ver == Ver::Http2; let connector = self.connector.clone(); - let dst = Destination { - uri, - }; + let dst = uri; hyper_lazy(move || { // Try to take a "connecting lock". // diff --git a/tests/client.rs b/tests/client.rs index b4b82b6818..567728ac3f 100644 --- a/tests/client.rs +++ b/tests/client.rs @@ -940,11 +940,12 @@ mod dispatch_impl { use futures_channel::{mpsc, oneshot}; use futures_util::future::{FutureExt, TryFutureExt}; use futures_util::stream::{StreamExt}; + use http::Uri; use tokio::runtime::Runtime; use tokio::io::{AsyncRead, AsyncWrite}; use tokio::net::TcpStream; - use hyper::client::connect::{Connected, Destination, HttpConnector}; + use hyper::client::connect::{Connected, HttpConnector}; use hyper::Client; #[test] @@ -1738,19 +1739,19 @@ mod dispatch_impl { } } - impl hyper::service::Service for DebugConnector { + impl hyper::service::Service for DebugConnector { type Response = (DebugStream, Connected); - type Error = >::Error; + type Error = >::Error; type Future = Pin > + Send>>; fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll> { // don't forget to check inner service is ready :) - hyper::service::Service::::poll_ready(&mut self.http, cx) + hyper::service::Service::::poll_ready(&mut self.http, cx) } - fn call(&mut self, dst: Destination) -> Self::Future { + fn call(&mut self, dst: Uri) -> Self::Future { self.connects.fetch_add(1, Ordering::SeqCst); let closes = self.closes.clone(); let is_proxy = self.is_proxy;