Shifts to a series of traits/associated types instead of having a requester

components/net/http_loader.rs

@@ -28,7 +28,7 @@ use hyper::Error as HttpError;
 use hyper::method::Method;
 use hyper::http::RawStatus;
 use hyper::mime::{Mime, TopLevel, SubLevel};
-use hyper::net::{Fresh, Streaming, HttpsConnector, Openssl, NetworkConnector, NetworkStream};
+use hyper::net::{Fresh, HttpsConnector, Openssl};
 use hyper::status::{StatusCode, StatusClass};
 use ipc_channel::ipc::{self, IpcSender};
 use log;
@@ -110,9 +110,7 @@ fn load_for_consumer(load_data: LoadData,
         devtools_chan: Option<Sender<DevtoolsControlMsg>>,
         hsts_list: Arc<Mutex<HSTSList>>) {
 
-    let requester = NetworkHttpRequester::new();
-
-    match load(load_data, resource_mgr_chan, devtools_chan, hsts_list, &requester) {
+    match load::<WrappedHttpRequest>(load_data, resource_mgr_chan, devtools_chan, hsts_list, &NetworkHttpRequestFactory) {
         Err(LoadError::UnsupportedScheme(url)) => {
             let s = format!("{} request, but we don't support that scheme", &*url.scheme);
             send_error(url, s, start_chan)
@@ -144,27 +142,6 @@ fn load_for_consumer(load_data: LoadData,
     }
 }
 
-struct NetworkHttpRequester {
-    connector: HttpsConnector<Openssl>
-}
-
-impl NetworkHttpRequester {
-    fn new() -> NetworkHttpRequester {
-        // 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) })
-        }
-    }
-}
-
 pub trait HttpResponse: Read {
     fn headers(&self) -> &Headers;
     fn status(&self) -> StatusCode;
@@ -196,16 +173,31 @@ impl HttpResponse for WrappedHttpResponse {
     }
 }
 
-impl NetworkHttpRequester {
-    fn start_connection(&self, url: &Url, method: &Method) -> Result<Request<Fresh>, LoadError> {
-        let connection = Request::with_connector(method.clone(), url.clone(), &self.connector);
+pub trait HttpRequestFactory {
+    type R: HttpRequest;
+
+    fn create(&self, url: Url, method: Method) -> Result<Self::R, LoadError>;
+}
+
+struct NetworkHttpRequestFactory;
+
+impl HttpRequestFactory for NetworkHttpRequestFactory {
+    type R = WrappedHttpRequest;
+
+    fn create(&self, url: Url, method: Method) -> Result<WrappedHttpRequest, LoadError> {
+        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();
+
+        let connector = HttpsConnector::new(Openssl { context: Arc::new(context) });
+        let connection = Request::with_connector(method.clone(), url.clone(), &connector);
 
         let ssl_err_string = "Some(OpenSslErrors([UnknownError { library: \"SSL routines\", \
     function: \"SSL3_GET_SERVER_CERTIFICATE\", \
     reason: \"certificate verify failed\" }]))";
 
-        match connection {
-            Ok(req) => Ok(req),
+        let request = match connection {
+            Ok(req) => req,
 
             Err(HttpError::Io(ref io_error)) if (
                 io_error.kind() == io::ErrorKind::Other &&
@@ -223,28 +215,38 @@ impl NetworkHttpRequester {
             Err(e) => {
                  return Err(LoadError::Connection(url.clone(), e.description().to_string()))
             }
-        }
+        };
+
+        Ok(WrappedHttpRequest { request: request })
     }
 }
 
-impl HttpRequester for NetworkHttpRequester {
-    fn send(&self, url: &Url, method: &Method, headers: &Headers, body: &Option<Vec<u8>>) -> Result<Box<HttpResponse>, LoadError> {
-        let mut request = try!(self.start_connection(url, method));
+pub trait HttpRequest {
+    type R: HttpResponse + 'static;
 
-        *request.headers_mut() = headers.clone();
+    fn headers_mut(&mut self) -> &mut Headers;
+    fn send(self, body: &Option<Vec<u8>>) -> Result<Self::R, LoadError>;
+}
 
-        // TODO: fix HEAD method (don't write the body, and force content-length to 0)
-        if let Some(ref data) = *body {
-            request.headers_mut().set(ContentLength(data.len() as u64));
-        }
+struct WrappedHttpRequest {
+    request: Request<Fresh>
+}
 
-        let mut request_writer = match request.start() {
+impl HttpRequest for WrappedHttpRequest {
+    type R = WrappedHttpResponse;
+
+    fn headers_mut(&mut self) -> &mut Headers {
+        self.request.headers_mut()
+    }
+
+    fn send(self, body: &Option<Vec<u8>>) -> Result<WrappedHttpResponse, LoadError> {
+        let mut request_writer = match self.request.start() {
             Ok(streaming) => streaming,
             Err(e) => return Err(LoadError::Connection(Url::parse("http://example.com").unwrap(), e.description().to_string()))
         };
 
         if let Some(ref data) = *body {
-            match request_writer.write_all(&*data) {
+            match request_writer.write_all(&data) {
                 Err(e) => {
                     return Err(LoadError::Connection(Url::parse("http://example.com").unwrap(), e.description().to_string()))
                 }
@@ -257,13 +259,8 @@ impl HttpRequester for NetworkHttpRequester {
             Err(e) => return Err(LoadError::Connection(Url::parse("http://example.com").unwrap(), e.description().to_string()))
         };
 
-        Ok(Box::new(WrappedHttpResponse { response: response }))
+        Ok(WrappedHttpResponse { response: response })
     }
-
-}
-
-pub trait HttpRequester {
-    fn send(&self, url: &Url, method: &Method, headers: &Headers, body: &Option<Vec<u8>>) -> Result<Box<HttpResponse>, LoadError>;
 }
 
 #[derive(Debug)]
@@ -277,12 +274,12 @@ pub enum LoadError {
     MaxRedirects(Url)
 }
 
-pub fn load(mut load_data: LoadData,
+pub fn load<A>(mut load_data: LoadData,
             resource_mgr_chan: IpcSender<ControlMsg>,
             devtools_chan: Option<Sender<DevtoolsControlMsg>>,
             hsts_list: Arc<Mutex<HSTSList>>,
-            requester: &HttpRequester)
-    -> Result<(Box<Read>, Metadata), LoadError> {
+            request_factory: &HttpRequestFactory<R=A>)
+    -> Result<(Box<Read>, Metadata), LoadError> where A: HttpRequest + 'static {
     // 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.
@@ -374,17 +371,24 @@ pub fn load(mut load_data: LoadData,
             request_headers.set_raw("Accept-Encoding".to_owned(), vec![b"gzip, deflate".to_vec()]);
         }
 
+        // --- Start sending the request
+        // TODO: Avoid automatically sending request body if a redirect has occurred.
+        let mut req = try!(request_factory.create(url.clone(), load_data.method.clone()));
+        *req.headers_mut() = request_headers;
+
         if log_enabled!(log::LogLevel::Info) {
             info!("{}", load_data.method);
-            for header in request_headers.iter() {
+            for header in req.headers_mut().iter() {
                 info!(" - {}", header);
             }
             info!("{:?}", load_data.data);
         }
 
-        // --- Start sending the request
-        // TODO: Avoid automatically sending request body if a redirect has occurred.
-        let response = try!(requester.send(&url, &load_data.method, &request_headers, &load_data.data));
+        if let Some(ref data) = load_data.data {
+            req.headers_mut().set(ContentLength(data.len() as u64));
+        }
+
+        let response = try!(req.send(&load_data.data));
 
         // --- Tell devtools we've made a request
         // Send an HttpRequest message to devtools with a unique request_id
@@ -401,7 +405,7 @@ pub fn load(mut load_data: LoadData,
         }
 
         // Dump headers, but only do the iteration if info!() is enabled.
-        info!("got HTTP response {}, headers:", (*response).status());
+        info!("got HTTP response {}, headers:", response.status());
         if log_enabled!(log::LogLevel::Info) {
             for header in response.headers().iter() {
                 info!(" - {}", header);

tests/unit/net/http_loader.rs

@@ -2,85 +2,30 @@
  * 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, HttpRequester, HttpResponse};
+use net::http_loader::{load, LoadError, HttpRequestFactory, HttpRequest, HttpResponse};
 use net::resource_task::new_resource_task;
 use url::Url;
 use std::sync::{Arc, Mutex};
 use ipc_channel::ipc;
 use net_traits::LoadData;
 use net::hsts::HSTSList;
-use hyper::client::Response;
 use hyper::method::Method;
 use hyper::http::RawStatus;
 use hyper::status::StatusCode;
 use hyper::header::{Headers, Location, ContentLength};
 use std::io::{self, Read};
 use std::cmp::{self};
-use std::mem::{self};
 use std::borrow::Cow;
 
-fn redirect_to(host: &str) -> Box<HttpResponse> {
-    let mut headers = Headers::new();
-    headers.set(Location(host.to_string()));
-
-    struct Redirect {
-        h: Headers,
-        sr: RawStatus
-    }
-
-    impl Read for Redirect {
-        fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
-            Ok(0)
-        }
-    }
-
-    impl HttpResponse for Redirect {
-        fn headers(&self) -> &Headers { &self.h }
-        fn status(&self) -> StatusCode { StatusCode::MovedPermanently }
-        fn status_raw(&self) -> &RawStatus { &self.sr }
-    }
-
-    Box::new(
-        Redirect {
-            h: headers,
-            sr: RawStatus(301, Cow::Borrowed("Moved Permanently"))
-        }
-    )
-}
-
-fn respond_with(body: &str) -> Box<HttpResponse> {
+fn respond_with(body: Vec<u8>) -> MockResponse {
     let mut headers = Headers::new();
     headers.set(ContentLength(body.len() as u64));
 
-    struct TextResponse {
-        h: Headers,
-        body: Vec<u8>,
-        sr: RawStatus
-    }
-
-    impl Read for TextResponse {
-        fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
-            let buf_len = buf.len();
-            for (a, b) in buf.iter_mut().zip(&self.body[0 .. cmp::min(buf_len, self.body.len())]) {
-                *a = *b
-            }
-
-            Ok(cmp::min(buf.len(), self.body.len()))
-        }
-    }
-
-    impl HttpResponse for TextResponse {
-        fn headers(&self) -> &Headers { &self.h }
-        fn status(&self) -> StatusCode { StatusCode::Ok }
-        fn status_raw(&self) -> &RawStatus { &self.sr }
-    }
-
-    Box::new(
-        TextResponse {
-            h: headers,
-            body: <[_]>::to_vec(body.as_bytes()),
-            sr: RawStatus(200, Cow::Borrowed("Ok"))
-        }
+    MockResponse::new(
+        headers,
+        StatusCode::Ok,
+        RawStatus(200, Cow::Borrowed("Ok")),
+        body
     )
 }
 
@@ -96,16 +41,94 @@ fn read_response(reader: &mut Read) -> String {
     }
 }
 
+struct MockResponse {
+    h: Headers,
+    sc: StatusCode,
+    sr: RawStatus,
+    msg: Vec<u8>
+}
+
+impl MockResponse {
+    fn new(h: Headers, sc: StatusCode, sr: RawStatus, msg: Vec<u8>) -> MockResponse {
+        MockResponse { h: h, sc: sc, sr: sr, msg: msg }
+    }
+}
+
+impl Read for MockResponse {
+    fn read(&mut self, buf: &mut [u8]) -> io::Result<usize> {
+        let buf_len = buf.len();
+        for (a, b) in buf.iter_mut().zip(&self.msg[0 .. cmp::min(buf_len, self.msg.len())]) {
+            *a = *b
+        }
+
+        Ok(cmp::min(buf.len(), self.msg.len()))
+    }
+}
+
+impl HttpResponse for MockResponse {
+    fn headers(&self) -> &Headers { &self.h }
+    fn status(&self) -> StatusCode { self.sc }
+    fn status_raw(&self) -> &RawStatus { &self.sr }
+}
+
+fn redirect_to(host: String) -> MockResponse {
+    let mut headers = Headers::new();
+    headers.set(Location(host.to_string()));
+
+    MockResponse::new(
+        headers,
+        StatusCode::MovedPermanently,
+        RawStatus(301, Cow::Borrowed("Moved Permanently")),
+        <[_]>::to_vec("".as_bytes())
+    )
+}
+
+
+enum RequestType {
+    Redirect(String),
+    Text(Vec<u8>)
+}
+
+struct MockRequest {
+    headers: Headers,
+    t: RequestType
+}
+
+impl MockRequest {
+    fn new(t: RequestType) -> MockRequest {
+        MockRequest { headers: Headers::new(), t: t }
+    }
+}
+
+impl HttpRequest for MockRequest {
+    type R=MockResponse;
+
+    fn headers_mut(&mut self) -> &mut Headers { &mut self.headers }
+
+    fn send(self, _: &Option<Vec<u8>>) -> Result<MockResponse, LoadError> {
+        match self.t {
+            RequestType::Redirect(location) => {
+                Ok(redirect_to(location))
+            },
+            RequestType::Text(b) => {
+                Ok(respond_with(b))
+            }
+        }
+    }
+}
+
 #[test]
 fn test_load_errors_when_there_a_redirect_loop() {
-    struct Redirector;
+    struct Factory;
 
-    impl HttpRequester for Redirector {
-        fn send(&self, url: &Url, _: &Method, _: &Headers, _: &Option<Vec<u8>>) -> Result<Box<HttpResponse>, LoadError> {
+    impl HttpRequestFactory for Factory {
+        type R=MockRequest;
+
+        fn create(&self, url: Url, _: Method) -> Result<MockRequest, LoadError> {
             if url.domain().unwrap() == "mozilla.com" {
-                Ok(redirect_to("http://mozilla.org"))
+                Ok(MockRequest::new(RequestType::Redirect("http://mozilla.org".to_string())))
             } else if url.domain().unwrap() == "mozilla.org" {
-                Ok(redirect_to("http://mozilla.com"))
+                Ok(MockRequest::new(RequestType::Redirect("http://mozilla.com".to_string())))
             } else {
                 panic!("unexpected host {:?}", url)
             }
@@ -117,7 +140,7 @@ fn test_load_errors_when_there_a_redirect_loop() {
     let load_data = LoadData::new(url.clone(), None);
     let hsts_list = Arc::new(Mutex::new(HSTSList { entries: Vec::new() }));
 
-    match load(load_data, resource_mgr, None, hsts_list, &Redirector) {
+    match load::<MockRequest>(load_data, resource_mgr, None, hsts_list, &Factory) {
         Err(LoadError::InvalidRedirect(_, msg)) => {
             assert_eq!(msg, "redirect loop");
         },
@@ -127,12 +150,14 @@ fn test_load_errors_when_there_a_redirect_loop() {
 
 #[test]
 fn test_load_errors_when_there_is_too_many_redirects() {
-    struct Redirector;
+    struct Factory;
 
-    impl HttpRequester for Redirector {
-        fn send(&self, url: &Url, _: &Method, _: &Headers, _: &Option<Vec<u8>>) -> Result<Box<HttpResponse>, LoadError> {
+    impl HttpRequestFactory for Factory {
+        type R=MockRequest;
+
+        fn create(&self, url: Url, _: Method) -> Result<MockRequest, LoadError> {
             if url.domain().unwrap() == "mozilla.com" {
-                Ok(redirect_to(&*format!("{}/1", url.serialize())))
+                Ok(MockRequest::new(RequestType::Redirect(format!("{}/1", url.serialize()))))
             } else {
                 panic!("unexpected host {:?}", url)
             }
@@ -144,7 +169,7 @@ fn test_load_errors_when_there_is_too_many_redirects() {
     let load_data = LoadData::new(url.clone(), None);
     let hsts_list = Arc::new(Mutex::new(HSTSList { entries: Vec::new() }));
 
-    match load(load_data, resource_mgr, None, hsts_list, &Redirector) {
+    match load::<MockRequest>(load_data, resource_mgr, None, hsts_list, &Factory) {
         Err(LoadError::MaxRedirects(url)) => {
             assert_eq!(url.domain().unwrap(), "mozilla.com")
         },
@@ -154,16 +179,24 @@ fn test_load_errors_when_there_is_too_many_redirects() {
 
 #[test]
 fn test_load_follows_a_redirect() {
-    struct Redirector;
+    struct Factory;
 
-    impl HttpRequester for Redirector {
-        fn send(&self, url: &Url, _: &Method, _: &Headers, _: &Option<Vec<u8>>) -> Result<Box<HttpResponse>, LoadError> {
+    impl HttpRequestFactory for Factory {
+        type R=MockRequest;
+
+        fn create(&self, url: Url, _: Method) -> Result<MockRequest, LoadError> {
             if url.domain().unwrap() == "mozilla.com" {
-                Ok(redirect_to("http://mozilla.org"))
+                Ok(MockRequest::new(RequestType::Redirect("http://mozilla.org".to_string())))
             } else if url.domain().unwrap() == "mozilla.org" {
-                Ok(respond_with("Yay!"))
+                Ok(
+                    MockRequest::new(
+                        RequestType::Text(
+                            <[_]>::to_vec("Yay!".as_bytes())
+                        )
+                    )
+                )
             } else {
-                panic!("unexpected host")
+                panic!("unexpected host {:?}", url)
             }
         }
     }
@@ -173,20 +206,22 @@ fn test_load_follows_a_redirect() {
     let load_data = LoadData::new(url.clone(), None);
     let hsts_list = Arc::new(Mutex::new(HSTSList { entries: Vec::new() }));
 
-    match load(load_data, resource_mgr, None, hsts_list, &Redirector) {
+    match load::<MockRequest>(load_data, resource_mgr, None, hsts_list, &Factory) {
         Err(_) => panic!("expected to follow a redirect"),
-        Ok((mut r, m)) => {
+        Ok((mut r, _)) => {
             let response = read_response(&mut *r);
             assert_eq!(response, "Yay!".to_string());
         }
     }
 }
 
-struct DontConnectHttpRequester;
+struct DontConnectFactory;
 
-impl HttpRequester for DontConnectHttpRequester {
-    fn send(&self, _: &Url, _: &Method, _: &Headers, _: &Option<Vec<u8>>) -> Result<Box<HttpResponse>, LoadError> {
-        Err(LoadError::Connection(Url::parse("http://example.com").unwrap(), "shouldn't connect".to_string()))
+impl HttpRequestFactory for DontConnectFactory {
+    type R=MockRequest;
+
+    fn create(&self, url: Url, _: Method) -> Result<MockRequest, LoadError> {
+        Err(LoadError::Connection(url, "should not have connected".to_string()))
     }
 }
 
@@ -197,7 +232,7 @@ fn test_load_errors_when_scheme_is_not_http_or_https() {
     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, &DontConnectHttpRequester) {
+    match load::<MockRequest>(load_data, cookies_chan, None, hsts_list, &DontConnectFactory) {
         Err(LoadError::UnsupportedScheme(_)) => {}
         _ => panic!("expected ftp scheme to be unsupported")
     }
@@ -210,7 +245,7 @@ fn test_load_errors_when_viewing_source_and_inner_url_scheme_is_not_http_or_http
     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, &DontConnectHttpRequester) {
+    match load::<MockRequest>(load_data, cookies_chan, None, hsts_list, &DontConnectFactory) {
         Err(LoadError::UnsupportedScheme(_)) => {}
         _ => panic!("expected ftp scheme to be unsupported")
     }