Abstracts the hyper connection to a servo HttpRequester trait

servo/servo#6727

components/net/http_loader.rs

@@ -21,6 +21,10 @@ use hyper::Error as HttpError;
 use hyper::client::Request;
 use hyper::header::StrictTransportSecurity;
 use hyper::header::{AcceptEncoding, Accept, ContentLength, ContentType, Host, Location, qitem, Quality, QualityItem};
+use hyper::client::{Request, Response};
+use hyper::header::{AcceptEncoding, Accept, ContentLength, ContentType, Host, Location, qitem, StrictTransportSecurity};
+use hyper::header::{Quality, QualityItem};
+use hyper::Error as HttpError;
 use hyper::method::Method;
 use hyper::mime::{Mime, TopLevel, SubLevel};
 use hyper::net::{Fresh, HttpsConnector, Openssl, NetworkConnector, NetworkStream};
@@ -105,20 +109,9 @@ fn load_for_consumer(load_data: LoadData,
         devtools_chan: Option<Sender<DevtoolsControlMsg>>,
         hsts_list: Arc<Mutex<HSTSList>>) {
 
-    let connector = {
+    let requester = NetworkHttpRequester::new();
-        // TODO: Is this still necessary? The type system is making it really hard to support both
-        // connectors. SSL is working, so it's not clear to me if the original rationale still
-        // stands?
-        // if opts::get().nossl {
-        //     &HttpConnector
-        let mut context = SslContext::new(SslMethod::Sslv23).unwrap();
-        context.set_verify(SSL_VERIFY_PEER, None);
-        context.set_CA_file(&resources_dir_path().join("certs")).unwrap();
 
-        &HttpsConnector::new(Openssl { context: Arc::new(context) })
+    match load(load_data, resource_mgr_chan, devtools_chan, hsts_list, &requester) {
-    };
-
-    match load(load_data, resource_mgr_chan, devtools_chan, hsts_list, connector) {
         Err(LoadError::UnsupportedScheme(url)) => {
             let s = format!("{} request, but we don't support that scheme", &*url.scheme);
             send_error(url, s, start_chan)
@@ -150,17 +143,38 @@ fn load_for_consumer(load_data: LoadData,
     }
 }
 
-#[inline(always)]
+struct NetworkHttpRequester {
-fn connect<C, S>(url: Url,
+    connector: HttpsConnector<Openssl>
-                 method: Method,
+}
-                 connector: &C) -> Result<Request<Fresh>, LoadError> where
+
-            C: NetworkConnector<Stream=S>,
+impl NetworkHttpRequester {
-            S: Into<Box<NetworkStream + Send>> {
+    fn new() -> NetworkHttpRequester {
-    let connection = Request::with_connector(method, url.clone(), connector);
+        // TODO: Is this still necessary? The type system is making it really hard to support both
+        // connectors. SSL is working, so it's not clear to me if the original rationale still
+        // stands?
+        // if opts::get().nossl {
+        //     &HttpConnector
+        let mut context = SslContext::new(SslMethod::Sslv23).unwrap();
+        context.set_verify(SSL_VERIFY_PEER, None);
+        context.set_CA_file(&resources_dir_path().join("certs")).unwrap();
+
+        NetworkHttpRequester {
+            connector: HttpsConnector::new(Openssl { context: Arc::new(context) })
+        }
+    }
+}
+
+impl HttpRequester for NetworkHttpRequester {
+    fn send(&self, request: Request<Fresh>) -> Result<Response, LoadError> {
+        unimplemented!()
+    }
+
+    fn build(&self, url: Url, method: Method) -> Result<Request<Fresh>, LoadError> {
+        let connection = Request::with_connector(method, url.clone(), &self.connector);
 
         let ssl_err_string = "Some(OpenSslErrors([UnknownError { library: \"SSL routines\", \
-function: \"SSL3_GET_SERVER_CERTIFICATE\", \
+    function: \"SSL3_GET_SERVER_CERTIFICATE\", \
-reason: \"certificate verify failed\" }]))";
+    reason: \"certificate verify failed\" }]))";
 
         match connection {
             Ok(req) => Ok(req),
@@ -182,6 +196,12 @@ reason: \"certificate verify failed\" }]))";
                  Err(LoadError::Connection(url, e.description().to_string()))
             }
         }
+    }
+}
+
+pub trait HttpRequester {
+    fn build(&self, url: Url, method: Method) -> Result<Request<Fresh>, LoadError>;
+    fn send(&self, request: Request<Fresh>) -> Result<Response, LoadError>;
 }
 
 pub enum LoadError {
@@ -194,14 +214,12 @@ pub enum LoadError {
     MaxRedirects(Url)
 }
 
-fn load<C, S>(mut load_data: LoadData,
+pub fn load(mut load_data: LoadData,
             resource_mgr_chan: IpcSender<ControlMsg>,
             devtools_chan: Option<Sender<DevtoolsControlMsg>>,
             hsts_list: Arc<Mutex<HSTSList>>,
-        connector: &C) -> Result<(Box<Read>, Metadata), LoadError> where
+            requester: &HttpRequester)
-            C: NetworkConnector<Stream=S>,
+    -> Result<(Box<Read>, Metadata), LoadError> {
-            S: Into<Box<NetworkStream + Send>> {
-
     // FIXME: At the time of writing this FIXME, servo didn't have any central
     //        location for configuration. If you're reading this and such a
     //        repository DOES exist, please update this constant to use it.
@@ -243,7 +261,7 @@ fn load<C, S>(mut load_data: LoadData,
 
         info!("requesting {}", url.serialize());
 
-        let mut req = try!(connect(url.clone(), load_data.method.clone(), connector));
+        let mut req = try!(requester.build(url.clone(), load_data.method.clone()));
 
         //Ensure that the host header is set from the original url
         let host = Host {

tests/unit/net/http_loader.rs

@@ -2,153 +2,25 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-use net::http_loader::{load, LoadError};
+use net::http_loader::{load, LoadError, HttpRequester};
-use hyper::net::{NetworkStream, NetworkConnector};
-use hyper;
 use url::Url;
-use std::io::{self, Read, Write, Cursor};
-use std::fmt;
-use std::net::SocketAddr;
 use std::sync::{Arc, Mutex};
 use ipc_channel::ipc;
 use net_traits::LoadData;
 use net::hsts::HSTSList;
-use net::hsts::HSTSEntry;
+use hyper::client::{Request, Response};
+use hyper::net::Fresh;
+use hyper::method::Method;
 
-#[derive(Clone)]
+struct MockHttpRequester;
-pub struct MockStream {
-    pub read: Cursor<Vec<u8>>,
-    pub write: Vec<u8>,
-    #[cfg(feature = "timeouts")]
-    pub read_timeout: Cell<Option<Duration>>,
-    #[cfg(feature = "timeouts")]
-    pub write_timeout: Cell<Option<Duration>>
-}
 
-impl fmt::Debug for MockStream {
+impl HttpRequester for MockHttpRequester {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+    fn build(&self, url: Url, _: Method) -> Result<Request<Fresh>, LoadError> {
-        write!(f, "MockStream {{ read: {:?}, write: {:?} }}", self.read.get_ref(), self.write)
+        Err(LoadError::Connection(url.clone(), "shouldn't connect".to_string()))
-    }
-}
-
-impl PartialEq for MockStream {
-    fn eq(&self, other: &MockStream) -> bool {
-        self.read.get_ref() == other.read.get_ref() && self.write == other.write
-    }
-}
-
-impl MockStream {
-    pub fn new() -> MockStream {
-        MockStream::with_input(b"")
     }
 
-    #[cfg(not(feature = "timeouts"))]
+    fn send(&self, _: Request<Fresh>) -> Result<Response, LoadError> {
-    pub fn with_input(input: &[u8]) -> MockStream {
+        Err(LoadError::Connection(Url::parse("http://example.com").unwrap(), "shouldn't connect".to_string()))
-        MockStream {
-            read: Cursor::new(input.to_vec()),
-            write: vec![]
-        }
-    }
-
-    #[cfg(feature = "timeouts")]
-    pub fn with_input(input: &[u8]) -> MockStream {
-        MockStream {
-            read: Cursor::new(input.to_vec()),
-            write: vec![],
-            read_timeout: Cell::new(None),
-            write_timeout: Cell::new(None),
-        }
-    }
-}
-
-impl Read for MockStream {
-    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
-        self.read.read(buf)
-    }
-}
-
-impl Write for MockStream {
-    fn write(&mut self, msg: &[u8]) -> io::Result<usize> {
-        Write::write(&mut self.write, msg)
-    }
-
-    fn flush(&mut self) -> io::Result<()> {
-        Ok(())
-    }
-}
-
-impl NetworkStream for MockStream {
-    fn peer_addr(&mut self) -> io::Result<SocketAddr> {
-        Ok("127.0.0.1:1337".parse().unwrap())
-    }
-
-    #[cfg(feature = "timeouts")]
-    fn set_read_timeout(&self, dur: Option<Duration>) -> io::Result<()> {
-        self.read_timeout.set(dur);
-        Ok(())
-    }
-
-    #[cfg(feature = "timeouts")]
-    fn set_write_timeout(&self, dur: Option<Duration>) -> io::Result<()> {
-        self.write_timeout.set(dur);
-        Ok(())
-    }
-}
-
-/// A wrapper around a `MockStream` that allows one to clone it and keep an independent copy to the
-/// same underlying stream.
-#[derive(Clone)]
-pub struct CloneableMockStream {
-    pub inner: Arc<Mutex<MockStream>>,
-}
-
-impl Write for CloneableMockStream {
-    fn write(&mut self, msg: &[u8]) -> io::Result<usize> {
-        self.inner.lock().unwrap().write(msg)
-    }
-
-    fn flush(&mut self) -> io::Result<()> {
-        self.inner.lock().unwrap().flush()
-    }
-}
-
-impl Read for CloneableMockStream {
-    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
-        self.inner.lock().unwrap().read(buf)
-    }
-}
-
-impl NetworkStream for CloneableMockStream {
-    fn peer_addr(&mut self) -> io::Result<SocketAddr> {
-        self.inner.lock().unwrap().peer_addr()
-    }
-
-    #[cfg(feature = "timeouts")]
-    fn set_read_timeout(&self, dur: Option<Duration>) -> io::Result<()> {
-        self.inner.lock().unwrap().set_read_timeout(dur)
-    }
-
-    #[cfg(feature = "timeouts")]
-    fn set_write_timeout(&self, dur: Option<Duration>) -> io::Result<()> {
-        self.inner.lock().unwrap().set_write_timeout(dur)
-    }
-}
-
-impl CloneableMockStream {
-    pub fn with_stream(stream: MockStream) -> CloneableMockStream {
-        CloneableMockStream {
-            inner: Arc::new(Mutex::new(stream)),
-        }
-    }
-}
-
-pub struct MockConnector;
-
-impl NetworkConnector for MockConnector {
-    type Stream = MockStream;
-
-    fn connect(&self, _host: &str, _port: u16, _scheme: &str) -> hyper::Result<MockStream> {
-        Ok(MockStream::new())
     }
 }
 
@@ -156,10 +28,10 @@ impl NetworkConnector for MockConnector {
 fn test_load_errors_when_scheme_is_not_http_or_https() {
     let url = Url::parse("ftp://not-supported").unwrap();
     let (cookies_chan, _) = ipc::channel().unwrap();
-    let load_data = LoadData::new(url, None);
+    let load_data = LoadData::new(url.clone(), None);
     let hsts_list = Arc::new(Mutex::new(HSTSList { entries: Vec::new() }));
 
-    match load(load_data, cookies_chan, None, hsts_list, &MockConnector) {
+    match load(load_data, cookies_chan, None, hsts_list, &MockHttpRequester) {
         Err(LoadError::UnsupportedScheme(_)) => {}
         _ => panic!("expected ftp scheme to be unsupported")
     }
@@ -169,10 +41,10 @@ fn test_load_errors_when_scheme_is_not_http_or_https() {
 fn test_load_errors_when_viewing_source_and_inner_url_scheme_is_not_http_or_https() {
     let url = Url::parse("view-source:ftp://not-supported").unwrap();
     let (cookies_chan, _) = ipc::channel().unwrap();
-    let load_data = LoadData::new(url, None);
+    let load_data = LoadData::new(url.clone(), None);
     let hsts_list = Arc::new(Mutex::new(HSTSList { entries: Vec::new() }));
 
-    match load(load_data, cookies_chan, None, hsts_list, &MockConnector) {
+    match load(load_data, cookies_chan, None, hsts_list, &MockHttpRequester) {
         Err(LoadError::UnsupportedScheme(_)) => {}
         _ => panic!("expected ftp scheme to be unsupported")
     }