From ab49393c3ce08d8599b3297c951dfa67d5d4320e Mon Sep 17 00:00:00 2001 From: Corey Farwell Date: Mon, 25 Apr 2016 14:37:44 -0700 Subject: [PATCH] servo: Merge #10785 - Refactor `LoadErrorType` to not require a `String` for every type (from frewsxcv:loaderrortype-nostring); r=jdm Some of the `LoadErrorType` like `LoadCancelled` don't need a `String` associated with the type since the variant is self-explanatory. There are some variants that don't need an associated `String`, but that can be cleaned up in a later refactor. Also, `net_traits::NetworkError` currently requires a `String`, but that can potentially also be refactored away too. Source-Repo: https://github.com/servo/servo Source-Revision: 81f6e70a623a6f11535322ed2ef954eafaf8c70c --- servo/components/net/http_loader.rs | 98 ++++++++++++++++++----------- servo/tests/unit/net/http_loader.rs | 9 ++- 2 files changed, 64 insertions(+), 43 deletions(-) diff --git a/servo/components/net/http_loader.rs b/servo/components/net/http_loader.rs index 1a53e013ad54..fedc31963974 100644 --- a/servo/components/net/http_loader.rs +++ b/servo/components/net/http_loader.rs @@ -36,6 +36,7 @@ use std::borrow::ToOwned; use std::boxed::FnBox; use std::collections::HashSet; use std::error::Error; +use std::fmt; use std::io::{self, Read, Write}; use std::sync::mpsc::Sender; use std::sync::{Arc, RwLock}; @@ -155,11 +156,11 @@ fn load_for_consumer(load_data: LoadData, user_agent, &cancel_listener) { Err(error) => { match error.error { - LoadErrorType::ConnectionAborted => unreachable!(), - LoadErrorType::Ssl => send_error(error.url.clone(), - NetworkError::SslValidation(error.url), - start_chan), - _ => send_error(error.url, NetworkError::Internal(error.reason), start_chan) + LoadErrorType::ConnectionAborted { .. } => unreachable!(), + LoadErrorType::Ssl { .. } => send_error(error.url.clone(), + NetworkError::SslValidation(error.url), + start_chan), + _ => send_error(error.url, NetworkError::Internal(error.error.description().to_owned()), start_chan) } } Ok(mut load_response) => { @@ -247,14 +248,15 @@ impl HttpRequestFactory for NetworkHttpRequestFactory { if let Some(&SslError::OpenSslErrors(ref errors)) = error.downcast_ref::() { if errors.iter().any(is_cert_verify_error) { let msg = format!("ssl error: {:?} {:?}", error.description(), error.cause()); - return Err(LoadError::new(url, LoadErrorType::Ssl, msg)); + return Err(LoadError::new(url, LoadErrorType::Ssl { reason: msg })); } } } let mut request = match connection { Ok(req) => req, - Err(e) => return Err(LoadError::new(url, LoadErrorType::Connection, e.description().to_owned())), + Err(e) => return Err( + LoadError::new(url, LoadErrorType::Connection { reason: e.description().to_owned() })), }; *request.headers_mut() = headers; @@ -279,23 +281,22 @@ impl HttpRequest for WrappedHttpRequest { let url = self.request.url.clone(); let mut request_writer = match self.request.start() { Ok(streaming) => streaming, - Err(e) => return Err(LoadError::new(url, LoadErrorType::Connection, e.description().to_owned())), + Err(e) => return Err(LoadError::new(url, LoadErrorType::Connection { reason: e.description().to_owned() })), }; if let Some(ref data) = *body { if let Err(e) = request_writer.write_all(&data) { - return Err(LoadError::new(url, LoadErrorType::Connection, e.description().to_owned())) + return Err(LoadError::new(url, LoadErrorType::Connection { reason: e.description().to_owned() })) } } let response = match request_writer.send() { Ok(w) => w, Err(HttpError::Io(ref io_error)) if io_error.kind() == io::ErrorKind::ConnectionAborted => { - return Err(LoadError::new(url, LoadErrorType::ConnectionAborted, - io_error.description().to_owned())); + let error_type = LoadErrorType::ConnectionAborted { reason: io_error.description().to_owned() }; + return Err(LoadError::new(url, error_type)); }, - Err(e) => return Err(LoadError::new(url, LoadErrorType::Connection, - e.description().to_owned())), + Err(e) => return Err(LoadError::new(url, LoadErrorType::Connection { reason: e.description().to_owned() })), }; Ok(WrappedHttpResponse { response: response }) @@ -306,15 +307,13 @@ impl HttpRequest for WrappedHttpRequest { pub struct LoadError { pub url: Url, pub error: LoadErrorType, - pub reason: String, } impl LoadError { - pub fn new(url: Url, error: LoadErrorType, reason: String) -> LoadError { + pub fn new(url: Url, error: LoadErrorType) -> LoadError { LoadError { url: url, error: error, - reason: reason, } } } @@ -322,14 +321,39 @@ impl LoadError { #[derive(Eq, PartialEq, Debug)] pub enum LoadErrorType { Cancelled, - Connection, - ConnectionAborted, - Cors, - Decoding, - InvalidRedirect, + Connection { reason: String }, + ConnectionAborted { reason: String }, + // Preflight fetch inconsistent with main fetch + CorsPreflightFetchInconsistent, + Decoding { reason: String }, + InvalidRedirect { reason: String }, MaxRedirects(u32), // u32 indicates number of redirects that occurred - Ssl, - UnsupportedScheme, + RedirectLoop, + Ssl { reason: String }, + UnsupportedScheme { scheme: String }, +} + +impl fmt::Display for LoadErrorType { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{}", self.description()) + } +} + +impl Error for LoadErrorType { + fn description(&self) -> &str { + match *self { + LoadErrorType::Cancelled => "load cancelled", + LoadErrorType::Connection { ref reason } => reason, + LoadErrorType::ConnectionAborted { ref reason } => reason, + LoadErrorType::CorsPreflightFetchInconsistent => "preflight fetch inconsistent with main fetch", + LoadErrorType::Decoding { ref reason } => reason, + LoadErrorType::InvalidRedirect { ref reason } => reason, + LoadErrorType::MaxRedirects(_) => "too many redirects", + LoadErrorType::RedirectLoop => "redirect loop", + LoadErrorType::Ssl { ref reason } => reason, + LoadErrorType::UnsupportedScheme { .. } => "unsupported url scheme", + } + } } fn set_default_accept_encoding(headers: &mut Headers) { @@ -492,7 +516,7 @@ impl StreamedResponse { let result = GzDecoder::new(response); match result { Ok(response_decoding) => Ok(StreamedResponse::new(m, Decoder::Gzip(response_decoding))), - Err(err) => Err(LoadError::new(m.final_url, LoadErrorType::Decoding, err.to_string())), + Err(err) => Err(LoadError::new(m.final_url, LoadErrorType::Decoding { reason: err.to_string() })), } } Some(Encoding::Deflate) => { @@ -708,7 +732,7 @@ pub fn obtain_response(request_factory: &HttpRequestFactory, headers.clone())); if cancel_listener.is_cancelled() { - return Err(LoadError::new(connection_url.clone(), LoadErrorType::Cancelled, "load cancelled".to_owned())); + return Err(LoadError::new(connection_url.clone(), LoadErrorType::Cancelled)); } let maybe_response = req.send(request_body); @@ -724,8 +748,8 @@ pub fn obtain_response(request_factory: &HttpRequestFactory, response = match maybe_response { Ok(r) => r, Err(e) => { - if let LoadErrorType::ConnectionAborted = e.error { - debug!("connection aborted ({:?}), possibly stale, trying new connection", e.reason); + if let LoadErrorType::ConnectionAborted { reason } = e.error { + debug!("connection aborted ({:?}), possibly stale, trying new connection", reason); continue; } else { return Err(e) @@ -777,7 +801,7 @@ pub fn load(load_data: &LoadData, let mut new_auth_header: Option> = None; if cancel_listener.is_cancelled() { - return Err(LoadError::new(doc_url, LoadErrorType::Cancelled, "load cancelled".to_owned())); + return Err(LoadError::new(doc_url, LoadErrorType::Cancelled)); } // If the URL is a view-source scheme then the scheme data contains the @@ -799,17 +823,16 @@ pub fn load(load_data: &LoadData, } if iters > max_redirects { - return Err(LoadError::new(doc_url, LoadErrorType::MaxRedirects(iters - 1), - "too many redirects".to_owned())); + return Err(LoadError::new(doc_url, LoadErrorType::MaxRedirects(iters - 1))); } if !matches!(doc_url.scheme(), "http" | "https") { - let s = format!("{} request, but we don't support that scheme", doc_url.scheme()); - return Err(LoadError::new(doc_url, LoadErrorType::UnsupportedScheme, s)); + let scheme = doc_url.scheme().to_owned(); + return Err(LoadError::new(doc_url, LoadErrorType::UnsupportedScheme { scheme: scheme })); } if cancel_listener.is_cancelled() { - return Err(LoadError::new(doc_url, LoadErrorType::Cancelled, "load cancelled".to_owned())); + return Err(LoadError::new(doc_url, LoadErrorType::Cancelled)); } info!("requesting {}", doc_url); @@ -875,9 +898,7 @@ pub fn load(load_data: &LoadData, // CORS (https://fetch.spec.whatwg.org/#http-fetch, status section, point 9, 10) if let Some(ref c) = load_data.cors { if c.preflight { - return Err(LoadError::new(doc_url, - LoadErrorType::Cors, - "Preflight fetch inconsistent with main fetch".to_owned())); + return Err(LoadError::new(doc_url, LoadErrorType::CorsPreflightFetchInconsistent)); } else { // XXXManishearth There are some CORS-related steps here, // but they don't seem necessary until credentials are implemented @@ -886,7 +907,8 @@ pub fn load(load_data: &LoadData, let new_doc_url = match doc_url.join(&new_url) { Ok(u) => u, - Err(e) => return Err(LoadError::new(doc_url, LoadErrorType::InvalidRedirect, e.to_string())), + Err(e) => return Err( + LoadError::new(doc_url, LoadErrorType::InvalidRedirect { reason: e.to_string() })), }; // According to https://tools.ietf.org/html/rfc7231#section-6.4.2, @@ -898,7 +920,7 @@ pub fn load(load_data: &LoadData, } if redirected_to.contains(&new_doc_url) { - return Err(LoadError::new(doc_url, LoadErrorType::InvalidRedirect, "redirect loop".to_owned())); + return Err(LoadError::new(doc_url, LoadErrorType::RedirectLoop)); } info!("redirecting to {}", new_doc_url); diff --git a/servo/tests/unit/net/http_loader.rs b/servo/tests/unit/net/http_loader.rs index 83d21a66742a..a32df4781bf7 100644 --- a/servo/tests/unit/net/http_loader.rs +++ b/servo/tests/unit/net/http_loader.rs @@ -1082,8 +1082,7 @@ fn test_load_errors_when_there_a_redirect_loop() { match load(&load_data, &ui_provider, &http_state, None, &Factory, DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)) { - Err(ref load_err) if load_err.error == LoadErrorType::InvalidRedirect => - assert_eq!(&load_err.reason, "redirect loop"), + Err(ref load_err) if load_err.error == LoadErrorType::RedirectLoop => (), _ => panic!("expected max redirects to fail") } } @@ -1172,7 +1171,7 @@ impl HttpRequestFactory for DontConnectFactory { type R = MockRequest; fn create(&self, url: Url, _: Method, _: Headers) -> Result { - Err(LoadError::new(url, LoadErrorType::Connection, "should not have connected".to_owned())) + Err(LoadError::new(url, LoadErrorType::Connection { reason: "should not have connected".into() })) } } @@ -1190,7 +1189,7 @@ fn test_load_errors_when_scheme_is_not_http_or_https() { &DontConnectFactory, DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)) { - Err(ref load_err) if load_err.error == LoadErrorType::UnsupportedScheme => (), + Err(ref load_err) if load_err.error == LoadErrorType::UnsupportedScheme { scheme: "ftp".into() } => (), _ => panic!("expected ftp scheme to be unsupported") } } @@ -1209,7 +1208,7 @@ fn test_load_errors_when_viewing_source_and_inner_url_scheme_is_not_http_or_http &DontConnectFactory, DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)) { - Err(ref load_err) if load_err.error == LoadErrorType::UnsupportedScheme => (), + Err(ref load_err) if load_err.error == LoadErrorType::UnsupportedScheme { scheme: "ftp".into() } => (), _ => panic!("expected ftp scheme to be unsupported") } }