From 35cb07944cf98f26e308c944a361d6f4f2b94f01 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Mon, 13 May 2019 14:18:42 -0600 Subject: [PATCH 1/3] Give a better error message when crates.io requests time out crates.io is hosted on Heroku, which means we have a hard 30 second time limit on all requests. Typically this is only hit when someone is attempting to upload a crate so large that it would have been eventually rejected anyway, but it can also happen if a user is on a very slow internet connection. When this happens, the request is terminated by the platform and we have no control over the response that gets sent. This results in the user getting a very unhelpful error message from Cargo, and some generic error page HTML spat out into their terminal. We could work around this on our end by adding a 29 second timeout *somewhere else* in the stack, but we have a lot of layers that buffer requests to protect against slow client attacks, and it'd be a pretty decent amount of work. Since we eventually want to switch over to having Cargo do the S3 upload instead of us, it doesn't make sense to spend so much time on an error scenario that eventually will go away. I've tried to keep this uncoupled from crates.io as much as possible, since alternate registries might not be hosted on Heroku or have the same restricitions. But I figure "a 503 that took more than 30 seconds" is a safe bet on this being hit. If we're ok with coupling this to crates.io, I'd like to include "If your crate is less than 10MB you can email help@crates.io for assistance" in the error message. Ref https://github.com/rust-lang/crates.io/issues/1709 --- src/crates-io/lib.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/crates-io/lib.rs b/src/crates-io/lib.rs index 1b4d860d692..5213457d996 100644 --- a/src/crates-io/lib.rs +++ b/src/crates-io/lib.rs @@ -5,6 +5,7 @@ use std::collections::BTreeMap; use std::fs::File; use std::io::prelude::*; use std::io::Cursor; +use std::time::Instant; use curl::easy::{Easy, List}; use failure::bail; @@ -310,6 +311,7 @@ impl Registry { fn handle(handle: &mut Easy, read: &mut dyn FnMut(&mut [u8]) -> usize) -> Result { let mut headers = Vec::new(); let mut body = Vec::new(); + let started; { let mut handle = handle.transfer(); handle.read_function(|buf| Ok(read(buf)))?; @@ -321,6 +323,7 @@ fn handle(handle: &mut Easy, read: &mut dyn FnMut(&mut [u8]) -> usize) -> Result headers.push(String::from_utf8_lossy(data).into_owned()); true })?; + started = Instant::now(); handle.perform()?; } @@ -334,6 +337,10 @@ fn handle(handle: &mut Easy, read: &mut dyn FnMut(&mut [u8]) -> usize) -> Result match (handle.response_code()?, errors) { (0, None) | (200, None) => {}, + (503, _) if started.elapsed().as_secs() >= 29 => { + bail!("Request took more than 30 seconds to complete. If you're\ + trying to upload a crate it may be too large") + } (code, Some(errors)) => { let code = StatusCode::from_u16(code as _)?; bail!("api errors (status {}): {}", code, errors.join(", ")) From 6af6789004801aadae8e53aa43042a902f12dba2 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Tue, 14 May 2019 10:20:54 -0600 Subject: [PATCH 2/3] Make crates.io timeout error specific to crates.io --- src/cargo/ops/registry.rs | 7 +-- src/crates-io/lib.rs | 110 ++++++++++++++++++++------------------ 2 files changed, 60 insertions(+), 57 deletions(-) diff --git a/src/cargo/ops/registry.rs b/src/cargo/ops/registry.rs index 4cab5bb644f..7a367640891 100644 --- a/src/cargo/ops/registry.rs +++ b/src/cargo/ops/registry.rs @@ -129,12 +129,7 @@ fn verify_dependencies( // This extra hostname check is mostly to assist with testing, // but also prevents someone using `--index` to specify // something that points to crates.io. - let is_crates_io = registry - .host() - .to_url() - .map(|u| u.host_str() == Some("crates.io")) - .unwrap_or(false); - if registry_src.is_default_registry() || is_crates_io { + if registry_src.is_default_registry() || registry.host_is_crates_io() { bail!("crates cannot be published to crates.io with dependencies sourced from other\n\ registries either publish `{}` on crates.io or pull it into this repository\n\ and specify it with a path and version\n\ diff --git a/src/crates-io/lib.rs b/src/crates-io/lib.rs index 5213457d996..95571d7d368 100644 --- a/src/crates-io/lib.rs +++ b/src/crates-io/lib.rs @@ -13,6 +13,7 @@ use http::status::StatusCode; use serde::{Deserialize, Serialize}; use serde_json; use url::percent_encoding::{percent_encode, QUERY_ENCODE_SET}; +use url::Url; pub type Result = std::result::Result; @@ -142,6 +143,12 @@ impl Registry { &self.host } + pub fn host_is_crates_io(&self) -> bool { + Url::parse(self.host()) + .map(|u| u.host_str() == Some("crates.io")) + .unwrap_or(false) + } + pub fn add_owners(&mut self, krate: &str, owners: &[&str]) -> Result { let body = serde_json::to_string(&OwnersReq { users: owners })?; let body = self.put(&format!("/crates/{}/owners", krate), body.as_bytes())?; @@ -208,7 +215,7 @@ impl Registry { headers.append(&format!("Authorization: {}", token))?; self.handle.http_headers(headers)?; - let body = handle(&mut self.handle, &mut |buf| body.read(buf).unwrap_or(0))?; + let body = self.handle(&mut |buf| body.read(buf).unwrap_or(0))?; let response = if body.is_empty() { "{}".parse()? @@ -301,61 +308,62 @@ impl Registry { Some(mut body) => { self.handle.upload(true)?; self.handle.in_filesize(body.len() as u64)?; - handle(&mut self.handle, &mut |buf| body.read(buf).unwrap_or(0)) + self.handle(&mut |buf| body.read(buf).unwrap_or(0)) } - None => handle(&mut self.handle, &mut |_| 0), + None => self.handle(&mut |_| 0), } } -} - -fn handle(handle: &mut Easy, read: &mut dyn FnMut(&mut [u8]) -> usize) -> Result { - let mut headers = Vec::new(); - let mut body = Vec::new(); - let started; - { - let mut handle = handle.transfer(); - handle.read_function(|buf| Ok(read(buf)))?; - handle.write_function(|data| { - body.extend_from_slice(data); - Ok(data.len()) - })?; - handle.header_function(|data| { - headers.push(String::from_utf8_lossy(data).into_owned()); - true - })?; - started = Instant::now(); - handle.perform()?; - } - let body = match String::from_utf8(body) { - Ok(body) => body, - Err(..) => bail!("response body was not valid utf-8"), - }; - let errors = serde_json::from_str::(&body).ok().map(|s| { - s.errors.into_iter().map(|s| s.detail).collect::>() - }); - - match (handle.response_code()?, errors) { - (0, None) | (200, None) => {}, - (503, _) if started.elapsed().as_secs() >= 29 => { - bail!("Request took more than 30 seconds to complete. If you're\ - trying to upload a crate it may be too large") + fn handle(&mut self, read: &mut dyn FnMut(&mut [u8]) -> usize) -> Result { + let mut headers = Vec::new(); + let mut body = Vec::new(); + let started; + { + let mut handle = self.handle.transfer(); + handle.read_function(|buf| Ok(read(buf)))?; + handle.write_function(|data| { + body.extend_from_slice(data); + Ok(data.len()) + })?; + handle.header_function(|data| { + headers.push(String::from_utf8_lossy(data).into_owned()); + true + })?; + started = Instant::now(); + handle.perform()?; } - (code, Some(errors)) => { - let code = StatusCode::from_u16(code as _)?; - bail!("api errors (status {}): {}", code, errors.join(", ")) + + let body = match String::from_utf8(body) { + Ok(body) => body, + Err(..) => bail!("response body was not valid utf-8"), + }; + let errors = serde_json::from_str::(&body).ok().map(|s| { + s.errors.into_iter().map(|s| s.detail).collect::>() + }); + + match (self.handle.response_code()?, errors) { + (0, None) | (200, None) => {}, + (503, _) if started.elapsed().as_secs() >= 29 && self.host_is_crates_io() => { + bail!("Request timed out after 30 seconds. If you're trying to \ + upload a crate it may be too large. If the crate is under \ + 10MB in size, you can email help@crates.io for assistance.") + } + (code, Some(errors)) => { + let code = StatusCode::from_u16(code as _)?; + bail!("api errors (status {}): {}", code, errors.join(", ")) + } + (code, None) => bail!( + "failed to get a 200 OK response, got {}\n\ + headers:\n\ + \t{}\n\ + body:\n\ + {}", + code, + headers.join("\n\t"), + body, + ), } - (code, None) => bail!( - "failed to get a 200 OK response, got {}\n\ - headers:\n\ - \t{}\n\ - body:\n\ - {}", - code, - headers.join("\n\t"), - body, - ), - } - Ok(body) + Ok(body) + } } From 4d93f2d1c5b0cd4dc81eb264734e4031f68a76f4 Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Tue, 14 May 2019 15:47:05 -0600 Subject: [PATCH 3/3] Don't display the timeout error if the API returned errors https://github.com/rust-lang/cargo/pull/6936#discussion_r283922456 --- src/crates-io/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/crates-io/lib.rs b/src/crates-io/lib.rs index 95571d7d368..941b0d75a92 100644 --- a/src/crates-io/lib.rs +++ b/src/crates-io/lib.rs @@ -343,7 +343,7 @@ impl Registry { match (self.handle.response_code()?, errors) { (0, None) | (200, None) => {}, - (503, _) if started.elapsed().as_secs() >= 29 && self.host_is_crates_io() => { + (503, None) if started.elapsed().as_secs() >= 29 && self.host_is_crates_io() => { bail!("Request timed out after 30 seconds. If you're trying to \ upload a crate it may be too large. If the crate is under \ 10MB in size, you can email help@crates.io for assistance.")