Mirrors/servo
1
0
Fork 0
mirror of https://github.com/servo/servo.git synced 2025-06-20 15:18:58 +01:00
Code Issues Projects Releases Packages Wiki Activity

Auto merge of #10785 - frewsxcv:loaderrortype-nostring, r=jdm

Browse source 
Refactor `LoadErrorType` to not require a `String` for every type.

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.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10785)
<!-- Reviewable:end -->
This commit is contained in:
bors-servo 2016-04-25 14:37:44 -07:00
commit 81f6e70a62
2 changed files with 64 additions and 43 deletions
Download patch file Download diff file Expand all files Collapse all files

98
components/net/http_loader.rs
View file

@ -36,6 +36,7 @@ use std::borrow::ToOwned;
use std::boxed::FnBox; use std::boxed::FnBox;
use std::collections::HashSet; use std::collections::HashSet;
use std::error::Error; use std::error::Error;
use std::fmt;
use std::io::{self, Read, Write}; use std::io::{self, Read, Write};
use std::sync::mpsc::Sender; use std::sync::mpsc::Sender;
use std::sync::{Arc, RwLock}; use std::sync::{Arc, RwLock};
@ -155,11 +156,11 @@ fn load_for_consumer(load_data: LoadData,
user_agent, &cancel_listener) { user_agent, &cancel_listener) {
Err(error) => { Err(error) => {
match error.error { match error.error {
LoadErrorType::ConnectionAborted => unreachable!(), LoadErrorType::ConnectionAborted { .. } => unreachable!(),
LoadErrorType::Ssl => send_error(error.url.clone(), LoadErrorType::Ssl { .. } => send_error(error.url.clone(),
NetworkError::SslValidation(error.url), NetworkError::SslValidation(error.url),
start_chan), start_chan),
_ => send_error(error.url, NetworkError::Internal(error.reason), start_chan) _ => send_error(error.url, NetworkError::Internal(error.error.description().to_owned()), start_chan)
} }
} }
Ok(mut load_response) => { Ok(mut load_response) => {
@ -247,14 +248,15 @@ impl HttpRequestFactory for NetworkHttpRequestFactory {
if let Some(&SslError::OpenSslErrors(ref errors)) = error.downcast_ref::<SslError>() { if let Some(&SslError::OpenSslErrors(ref errors)) = error.downcast_ref::<SslError>() {
if errors.iter().any(is_cert_verify_error) { if errors.iter().any(is_cert_verify_error) {
let msg = format!("ssl error: {:?} {:?}", error.description(), error.cause()); 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 { let mut request = match connection {
Ok(req) => req, 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; *request.headers_mut() = headers;
@ -279,23 +281,22 @@ impl HttpRequest for WrappedHttpRequest {
let url = self.request.url.clone(); let url = self.request.url.clone();
let mut request_writer = match self.request.start() { let mut request_writer = match self.request.start() {
Ok(streaming) => streaming, 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 Some(ref data) = *body {
if let Err(e) = request_writer.write_all(&data) { 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() { let response = match request_writer.send() {
Ok(w) => w, Ok(w) => w,
Err(HttpError::Io(ref io_error)) if io_error.kind() == io::ErrorKind::ConnectionAborted => { Err(HttpError::Io(ref io_error)) if io_error.kind() == io::ErrorKind::ConnectionAborted => {
return Err(LoadError::new(url, LoadErrorType::ConnectionAborted, let error_type = LoadErrorType::ConnectionAborted { reason: io_error.description().to_owned() };
io_error.description().to_owned())); return Err(LoadError::new(url, error_type));
}, },
Err(e) => return Err(LoadError::new(url, LoadErrorType::Connection, Err(e) => return Err(LoadError::new(url, LoadErrorType::Connection { reason: e.description().to_owned() })),
e.description().to_owned())),
}; };
Ok(WrappedHttpResponse { response: response }) Ok(WrappedHttpResponse { response: response })
@ -306,15 +307,13 @@ impl HttpRequest for WrappedHttpRequest {
pub struct LoadError { pub struct LoadError {
pub url: Url, pub url: Url,
pub error: LoadErrorType, pub error: LoadErrorType,
pub reason: String,
} }
impl LoadError { impl LoadError {
pub fn new(url: Url, error: LoadErrorType, reason: String) -> LoadError { pub fn new(url: Url, error: LoadErrorType) -> LoadError {
LoadError { LoadError {
url: url, url: url,
error: error, error: error,
reason: reason,
} }
} }
} }
@ -322,14 +321,39 @@ impl LoadError {
#[derive(Eq, PartialEq, Debug)] #[derive(Eq, PartialEq, Debug)]
pub enum LoadErrorType { pub enum LoadErrorType {
Cancelled, Cancelled,
Connection, Connection { reason: String },
ConnectionAborted, ConnectionAborted { reason: String },
Cors, // Preflight fetch inconsistent with main fetch
Decoding, CorsPreflightFetchInconsistent,
InvalidRedirect, Decoding { reason: String },
InvalidRedirect { reason: String },
MaxRedirects(u32), // u32 indicates number of redirects that occurred MaxRedirects(u32), // u32 indicates number of redirects that occurred
Ssl, RedirectLoop,
UnsupportedScheme, 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) { fn set_default_accept_encoding(headers: &mut Headers) {
@ -492,7 +516,7 @@ impl<R: HttpResponse> StreamedResponse<R> {
let result = GzDecoder::new(response); let result = GzDecoder::new(response);
match result { match result {
Ok(response_decoding) => Ok(StreamedResponse::new(m, Decoder::Gzip(response_decoding))), 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) => { Some(Encoding::Deflate) => {
@ -708,7 +732,7 @@ pub fn obtain_response<A>(request_factory: &HttpRequestFactory<R=A>,
headers.clone())); headers.clone()));
if cancel_listener.is_cancelled() { 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); let maybe_response = req.send(request_body);
@ -724,8 +748,8 @@ pub fn obtain_response<A>(request_factory: &HttpRequestFactory<R=A>,
response = match maybe_response { response = match maybe_response {
Ok(r) => r, Ok(r) => r,
Err(e) => { Err(e) => {
if let LoadErrorType::ConnectionAborted = e.error { if let LoadErrorType::ConnectionAborted { reason } = e.error {
debug!("connection aborted ({:?}), possibly stale, trying new connection", e.reason); debug!("connection aborted ({:?}), possibly stale, trying new connection", reason);
continue; continue;
} else { } else {
return Err(e) return Err(e)
@ -777,7 +801,7 @@ pub fn load<A, B>(load_data: &LoadData,
let mut new_auth_header: Option<Authorization<Basic>> = None; let mut new_auth_header: Option<Authorization<Basic>> = None;
if cancel_listener.is_cancelled() { 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 // If the URL is a view-source scheme then the scheme data contains the
@ -799,17 +823,16 @@ pub fn load<A, B>(load_data: &LoadData,
} }
if iters > max_redirects { if iters > max_redirects {
return Err(LoadError::new(doc_url, LoadErrorType::MaxRedirects(iters - 1), return Err(LoadError::new(doc_url, LoadErrorType::MaxRedirects(iters - 1)));
"too many redirects".to_owned()));
} }
if !matches!(doc_url.scheme(), "http" | "https") { if !matches!(doc_url.scheme(), "http" | "https") {
let s = format!("{} request, but we don't support that scheme", doc_url.scheme()); let scheme = doc_url.scheme().to_owned();
return Err(LoadError::new(doc_url, LoadErrorType::UnsupportedScheme, s)); return Err(LoadError::new(doc_url, LoadErrorType::UnsupportedScheme { scheme: scheme }));
} }
if cancel_listener.is_cancelled() { 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); info!("requesting {}", doc_url);
@ -875,9 +898,7 @@ pub fn load<A, B>(load_data: &LoadData,
// CORS (https://fetch.spec.whatwg.org/#http-fetch, status section, point 9, 10) // CORS (https://fetch.spec.whatwg.org/#http-fetch, status section, point 9, 10)
if let Some(ref c) = load_data.cors { if let Some(ref c) = load_data.cors {
if c.preflight { if c.preflight {
return Err(LoadError::new(doc_url, return Err(LoadError::new(doc_url, LoadErrorType::CorsPreflightFetchInconsistent));
LoadErrorType::Cors,
"Preflight fetch inconsistent with main fetch".to_owned()));
} else { } else {
// XXXManishearth There are some CORS-related steps here, // XXXManishearth There are some CORS-related steps here,
// but they don't seem necessary until credentials are implemented // but they don't seem necessary until credentials are implemented
@ -886,7 +907,8 @@ pub fn load<A, B>(load_data: &LoadData,
let new_doc_url = match doc_url.join(&new_url) { let new_doc_url = match doc_url.join(&new_url) {
Ok(u) => u, 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, // According to https://tools.ietf.org/html/rfc7231#section-6.4.2,
@ -898,7 +920,7 @@ pub fn load<A, B>(load_data: &LoadData,
} }
if redirected_to.contains(&new_doc_url) { 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); info!("redirecting to {}", new_doc_url);

9
tests/unit/net/http_loader.rs
View file

@ -1082,8 +1082,7 @@ fn test_load_errors_when_there_a_redirect_loop() {
match load(&load_data, &ui_provider, &http_state, None, &Factory, match load(&load_data, &ui_provider, &http_state, None, &Factory,
DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)) { DEFAULT_USER_AGENT.to_owned(), &CancellationListener::new(None)) {
Err(ref load_err) if load_err.error == LoadErrorType::InvalidRedirect => Err(ref load_err) if load_err.error == LoadErrorType::RedirectLoop => (),
assert_eq!(&load_err.reason, "redirect loop"),
_ => panic!("expected max redirects to fail") _ => panic!("expected max redirects to fail")
} }
} }
@ -1172,7 +1171,7 @@ impl HttpRequestFactory for DontConnectFactory {
type R = MockRequest; type R = MockRequest;
fn create(&self, url: Url, _: Method, _: Headers) -> Result<MockRequest, LoadError> { fn create(&self, url: Url, _: Method, _: Headers) -> Result<MockRequest, LoadError> {
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, &DontConnectFactory,
DEFAULT_USER_AGENT.to_owned(), DEFAULT_USER_AGENT.to_owned(),
&CancellationListener::new(None)) { &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") _ => 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, &DontConnectFactory,
DEFAULT_USER_AGENT.to_owned(), DEFAULT_USER_AGENT.to_owned(),
&CancellationListener::new(None)) { &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") _ => panic!("expected ftp scheme to be unsupported")
} }
} }