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.
2024-07-17 02:57:34 +00:00 · 2023-08-01 10:52:28 +02:00 · 2023-08-01 10:52:28 +02:00 · 5df2b0b4dc
parent a05e890d56
commit 5df2b0b4dc
3 changed files with 145 additions and 39 deletions
--- 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<u8>, 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(_))));
+  }
 }
--- 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]
--- 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;