From 70650b74277ab2ada43007e00263ed58ee68e5c8 Mon Sep 17 00:00:00 2001 From: Jake Shadle Date: Thu, 17 Oct 2024 11:04:22 +0200 Subject: [PATCH] Add basic HTTP retry due to EOF I/O errors --- src/ctx.rs | 143 ++++++++++++++++++++++++++++++-------- src/main.rs | 8 ++- tests/compiles.rs | 2 + tests/deterministic.rs | 1 + tests/snapshots/xwin.snap | 7 ++ 5 files changed, 129 insertions(+), 32 deletions(-) diff --git a/src/ctx.rs b/src/ctx.rs index 6c07523..1948bc5 100644 --- a/src/ctx.rs +++ b/src/ctx.rs @@ -21,10 +21,15 @@ pub struct Ctx { pub tempdir: Option, pub client: ureq::Agent, pub draw_target: ProgressTarget, + pub http_retry: u8, } impl Ctx { - pub fn with_temp(dt: ProgressTarget, client: ureq::Agent) -> Result { + pub fn with_temp( + dt: ProgressTarget, + client: ureq::Agent, + http_retry: u8, + ) -> Result { let td = tempfile::TempDir::new()?; Ok(Self { @@ -34,6 +39,7 @@ impl Ctx { tempdir: Some(td), client, draw_target: dt, + http_retry, }) } @@ -41,6 +47,7 @@ impl Ctx { mut work_dir: PathBuf, dt: ProgressTarget, client: ureq::Agent, + http_retry: u8, ) -> Result { work_dir.push("dl"); std::fs::create_dir_all(&work_dir)?; @@ -54,6 +61,7 @@ impl Ctx { tempdir: None, client, draw_target: dt, + http_retry, }) } @@ -62,7 +70,7 @@ impl Ctx { url: impl AsRef, path: &P, checksum: Option, - progress: indicatif::ProgressBar, + mut progress: indicatif::ProgressBar, ) -> Result where P: AsRef + std::fmt::Debug, @@ -107,25 +115,25 @@ impl Ctx { } } - let res = self.client.get(url.as_ref()).call()?; - - let content_length = res - .headers() - .get("content-length") - .and_then(|header| header.to_str().ok()?.parse().ok()) - .unwrap_or_default(); - progress.inc_length(content_length); - - let body = bytes::BytesMut::with_capacity(content_length as usize); + use bytes::BufMut; struct ProgressCopy { progress: indicatif::ProgressBar, inner: bytes::buf::Writer, + failed: usize, + written: usize, } impl std::io::Write for ProgressCopy { fn write(&mut self, buf: &[u8]) -> std::io::Result { - self.progress.inc(buf.len() as u64); + self.written += buf.len(); + if self.failed == 0 { + self.progress.inc(buf.len() as u64); + } else if self.written > self.failed { + self.progress.inc((self.written - self.failed) as u64); + self.failed = 0; + } + self.inner.write(buf) } @@ -134,32 +142,107 @@ impl Ctx { } } - use bytes::BufMut; + enum DownloadError { + Ureq(ureq::Error), + Io(std::io::Error), + Retry((bytes::BytesMut, indicatif::ProgressBar)), + } + + let try_download = |mut body: bytes::BytesMut, + progress: indicatif::ProgressBar| + -> Result { + let res = self + .client + .get(url.as_ref()) + .call() + .map_err(DownloadError::Ureq)?; + + let content_length = res + .headers() + .get("content-length") + .and_then(|header| header.to_str().ok()?.parse().ok()) + .unwrap_or_default(); + + if body.capacity() > 0 { + if body.capacity() as u64 != content_length { + tracing::warn!(url = url.as_ref(), "a previous HTTP GET had a content-length of {}, but we now received a content-length of {content_length}", body.capacity()); + + if body.capacity() as u64 > content_length { + progress.inc_length(body.capacity() as u64 - content_length); + } else { + body.reserve(content_length as usize - body.capacity()); + } + } + } else { + body.reserve(content_length as usize); + progress.inc_length(content_length); + } - let mut pc = ProgressCopy { - progress, - inner: body.writer(), + let failed = body.len(); + body.clear(); + + let mut pc = ProgressCopy { + progress, + inner: body.writer(), + failed, + written: 0, + }; + + let res = std::io::copy(&mut res.into_body().as_reader(), &mut pc); + let body = pc.inner.into_inner(); + + match res { + Ok(_) => Ok(body), + Err(ref err) if err.kind() == std::io::ErrorKind::UnexpectedEof => { + Err(DownloadError::Retry((body, pc.progress))) + } + Err(err) => Err(DownloadError::Io(err)), + } }; - std::io::copy(&mut res.into_body().as_reader(), &mut pc)?; + let mut tries = self.http_retry + 1; + let total = tries; + let mut body = bytes::BytesMut::new(); - let body = pc.inner.into_inner().freeze(); + while tries > 0 { + match try_download(body, progress) { + Ok(body) => { + let body = body.freeze(); - if let Some(expected) = checksum { - let chksum = Sha256::digest(&body); + if let Some(expected) = checksum { + let chksum = Sha256::digest(&body); - anyhow::ensure!( - chksum == expected, - "checksum mismatch, expected {expected} != actual {chksum}" - ); - } + anyhow::ensure!( + chksum == expected, + "checksum mismatch, expected {expected} != actual {chksum}" + ); + } - if let Some(parent) = cache_path.parent() { - std::fs::create_dir_all(parent)?; + if let Some(parent) = cache_path.parent() { + std::fs::create_dir_all(parent)?; + } + + std::fs::write(cache_path, &body)?; + return Ok(body); + } + Err(DownloadError::Retry((b, prog))) => { + tries -= 1; + body = b; + progress = prog; + continue; + } + Err(DownloadError::Ureq(err)) => { + return Err(err) + .with_context(|| format!("HTTP GET request for {} failed", url.as_ref())); + } + Err(DownloadError::Io(err)) => { + return Err(err) + .with_context(|| format!("failed to retrieve body for {}", url.as_ref())); + } + } } - std::fs::write(cache_path, &body)?; - Ok(body) + anyhow::bail!("failed to retrieve {} after {total} tries due to I/O failures reading the response body, try using --http-retries to increase the retry count", url.as_ref()); } #[allow(clippy::too_many_arguments)] diff --git a/src/main.rs b/src/main.rs index b56ecc0..9f364a1 100644 --- a/src/main.rs +++ b/src/main.rs @@ -230,6 +230,10 @@ pub struct Args { /// An HTTPS proxy to use #[arg(long, env = "HTTPS_PROXY")] https_proxy: Option, + /// The number of times an HTTP get will be retried if it fails due to I/O + /// failures + #[arg(long, env = "XWIN_HTTP_RETRY", default_value = "0")] + http_retry: u8, /// The architectures to include #[arg( long, @@ -287,13 +291,13 @@ fn main() -> Result<(), Error> { }; let ctx = if args.temp { - xwin::Ctx::with_temp(draw_target, client)? + xwin::Ctx::with_temp(draw_target, client, args.http_retry)? } else { let cache_dir = match &args.cache_dir { Some(cd) => cd.clone(), None => cwd.join(".xwin-cache"), }; - xwin::Ctx::with_dir(cache_dir, draw_target, client)? + xwin::Ctx::with_dir(cache_dir, draw_target, client, args.http_retry)? }; let ctx = std::sync::Arc::new(ctx); diff --git a/tests/compiles.rs b/tests/compiles.rs index e783d23..f9a1236 100644 --- a/tests/compiles.rs +++ b/tests/compiles.rs @@ -4,6 +4,7 @@ fn verify_compiles() { xwin::PathBuf::from(".xwin-cache/compile-test"), xwin::util::ProgressTarget::Hidden, ureq::agent(), + 0, ) .unwrap(); @@ -167,6 +168,7 @@ fn verify_compiles_minimized() { xwin::PathBuf::from(".xwin-cache/compile-test-minimized"), xwin::util::ProgressTarget::Hidden, ureq::agent(), + 0, ) .unwrap(); diff --git a/tests/deterministic.rs b/tests/deterministic.rs index 4f4960c..223d6f5 100644 --- a/tests/deterministic.rs +++ b/tests/deterministic.rs @@ -7,6 +7,7 @@ fn verify_deterministic() { PathBuf::from(".xwin-cache/deterministic"), xwin::util::ProgressTarget::Hidden, ureq::agent(), + 0, ) .unwrap(); diff --git a/tests/snapshots/xwin.snap b/tests/snapshots/xwin.snap index 68145c7..61ab56f 100644 --- a/tests/snapshots/xwin.snap +++ b/tests/snapshots/xwin.snap @@ -84,6 +84,13 @@ Options: [env: HTTPS_PROXY] + --http-retry + The number of times an HTTP get will be retried if it fails due to I/O + failures + + [env: XWIN_HTTP_RETRY] + [default: 0] + --arch The architectures to include