From 5df2b0b4dc17cdccaada24ab3993c8983cabc925 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= Date: Tue, 1 Aug 2023 10:52:28 +0200 Subject: [PATCH] fix: retry module download once if server errored (#17252) Closes https://github.com/denoland/deno/issues/17251 Closes #19970 This commits adds logic to retry failed module downloads once. Both request and server errors are handled and the retry is done after 50 ms wait time. --- cli/file_fetcher.rs | 177 ++++++++++++++---- .../testdata/cert/localhost_unsafe_ssl.ts.out | 2 +- test_util/src/lib.rs | 5 + 3 files changed, 145 insertions(+), 39 deletions(-) diff --git a/cli/file_fetcher.rs b/cli/file_fetcher.rs index 2426ed0a54..cebb22b209 100644 --- a/cli/file_fetcher.rs +++ b/cli/file_fetcher.rs @@ -383,44 +383,84 @@ impl FileFetcher { let specifier = specifier.clone(); let client = self.http_client.clone(); let file_fetcher = self.clone(); - // A single pass of fetch either yields code or yields a redirect. + // A single pass of fetch either yields code or yields a redirect, server + // error causes a single retry to avoid crashing hard on intermittent failures. + + async fn handle_request_or_server_error( + retried: &mut bool, + specifier: &Url, + err_str: String, + ) -> Result<(), AnyError> { + // Retry once, and bail otherwise. + if !*retried { + *retried = true; + log::debug!("Import '{}' failed: {}. Retrying...", specifier, err_str); + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + Ok(()) + } else { + Err(generic_error(format!( + "Import '{}' failed: {}", + specifier, err_str + ))) + } + } + async move { - let result = match fetch_once( - &client, - FetchOnceArgs { - url: specifier.clone(), - maybe_accept: maybe_accept.clone(), - maybe_etag, - maybe_auth_token, - maybe_progress_guard: maybe_progress_guard.as_ref(), - }, - ) - .await? - { - FetchOnceResult::NotModified => { - let file = file_fetcher.fetch_cached(&specifier, 10)?.unwrap(); - Ok(file) - } - FetchOnceResult::Redirect(redirect_url, headers) => { - file_fetcher.http_cache.set(&specifier, headers, &[])?; - file_fetcher - .fetch_remote( - &redirect_url, - permissions, - redirect_limit - 1, - maybe_accept, + let mut retried = false; + let result = loop { + let result = match fetch_once( + &client, + FetchOnceArgs { + url: specifier.clone(), + maybe_accept: maybe_accept.clone(), + maybe_etag: maybe_etag.clone(), + maybe_auth_token: maybe_auth_token.clone(), + maybe_progress_guard: maybe_progress_guard.as_ref(), + }, + ) + .await? + { + FetchOnceResult::NotModified => { + let file = file_fetcher.fetch_cached(&specifier, 10)?.unwrap(); + Ok(file) + } + FetchOnceResult::Redirect(redirect_url, headers) => { + file_fetcher.http_cache.set(&specifier, headers, &[])?; + file_fetcher + .fetch_remote( + &redirect_url, + permissions, + redirect_limit - 1, + maybe_accept, + ) + .await + } + FetchOnceResult::Code(bytes, headers) => { + file_fetcher + .http_cache + .set(&specifier, headers.clone(), &bytes)?; + let file = + file_fetcher.build_remote_file(&specifier, bytes, &headers)?; + Ok(file) + } + FetchOnceResult::RequestError(err) => { + handle_request_or_server_error(&mut retried, &specifier, err) + .await?; + continue; + } + FetchOnceResult::ServerError(status) => { + handle_request_or_server_error( + &mut retried, + &specifier, + status.to_string(), ) - .await - } - FetchOnceResult::Code(bytes, headers) => { - file_fetcher - .http_cache - .set(&specifier, headers.clone(), &bytes)?; - let file = - file_fetcher.build_remote_file(&specifier, bytes, &headers)?; - Ok(file) - } + .await?; + continue; + } + }; + break result; }; + drop(maybe_progress_guard); result } @@ -572,6 +612,8 @@ enum FetchOnceResult { Code(Vec, HeadersMap), NotModified, Redirect(Url, HeadersMap), + RequestError(String), + ServerError(StatusCode), } #[derive(Debug)] @@ -606,7 +648,15 @@ async fn fetch_once<'a>( let accepts_val = HeaderValue::from_str(&accept)?; request = request.header(ACCEPT, accepts_val); } - let response = request.send().await?; + let response = match request.send().await { + Ok(resp) => resp, + Err(err) => { + if err.is_connect() || err.is_timeout() { + return Ok(FetchOnceResult::RequestError(err.to_string())); + } + return Err(err.into()); + } + }; if response.status() == StatusCode::NOT_MODIFIED { return Ok(FetchOnceResult::NotModified); @@ -639,8 +689,13 @@ async fn fetch_once<'a>( return Ok(FetchOnceResult::Redirect(new_url, result_headers)); } - if response.status().is_client_error() || response.status().is_server_error() - { + let status = response.status(); + + if status.is_server_error() { + return Ok(FetchOnceResult::ServerError(status)); + } + + if status.is_client_error() { let err = if response.status() == StatusCode::NOT_FOUND { custom_error( "NotFound", @@ -2230,4 +2285,50 @@ mod tests { // Check that the error message contains the original URL assert!(err.to_string().contains(url_str)); } + + #[tokio::test] + async fn server_error() { + let _g = test_util::http_server(); + let url_str = "http://127.0.0.1:4545/server_error"; + let url = Url::parse(url_str).unwrap(); + let client = create_test_client(); + let result = fetch_once( + &client, + FetchOnceArgs { + url, + maybe_accept: None, + maybe_etag: None, + maybe_auth_token: None, + maybe_progress_guard: None, + }, + ) + .await; + + if let Ok(FetchOnceResult::ServerError(status)) = result { + assert_eq!(status, 500); + } else { + panic!(); + } + } + + #[tokio::test] + async fn request_error() { + let _g = test_util::http_server(); + let url_str = "http://127.0.0.1:9999/"; + let url = Url::parse(url_str).unwrap(); + let client = create_test_client(); + let result = fetch_once( + &client, + FetchOnceArgs { + url, + maybe_accept: None, + maybe_etag: None, + maybe_auth_token: None, + maybe_progress_guard: None, + }, + ) + .await; + + assert!(matches!(result, Ok(FetchOnceResult::RequestError(_)))); + } } diff --git a/cli/tests/testdata/cert/localhost_unsafe_ssl.ts.out b/cli/tests/testdata/cert/localhost_unsafe_ssl.ts.out index 4b95c1136a..81e490c1cd 100644 --- a/cli/tests/testdata/cert/localhost_unsafe_ssl.ts.out +++ b/cli/tests/testdata/cert/localhost_unsafe_ssl.ts.out @@ -1,3 +1,3 @@ DANGER: TLS certificate validation is disabled for: deno.land -error: error sending request for url (https://localhost:5545/subdir/mod2.ts): error trying to connect: invalid peer certificate: UnknownIssuer +error: Import 'https://localhost:5545/subdir/mod2.ts' failed: error sending request for url (https://localhost:5545/subdir/mod2.ts): error trying to connect: invalid peer certificate: UnknownIssuer at file:///[WILDCARD]/cafile_url_imports.ts:[WILDCARD] diff --git a/test_util/src/lib.rs b/test_util/src/lib.rs index dcf12d860e..9b84db7f36 100644 --- a/test_util/src/lib.rs +++ b/test_util/src/lib.rs @@ -808,6 +808,11 @@ async fn main_server( *res.status_mut() = StatusCode::FOUND; Ok(res) } + (_, "/server_error") => { + let mut res = Response::new(Body::empty()); + *res.status_mut() = StatusCode::INTERNAL_SERVER_ERROR; + Ok(res) + } (_, "/x_deno_warning.js") => { let mut res = Response::new(Body::empty()); *res.status_mut() = StatusCode::MOVED_PERMANENTLY;