fix(ext/fetch): include URL and error details on fetch failures (#24910)

This commit improves error messages that `fetch` generates on failure.

Fixes #24835

Cargo.lock

@@ -1493,6 +1493,7 @@ dependencies = [
  "deno_permissions",
  "deno_tls",
  "dyn-clone",
+ "error_reporter",
  "fast-socks5",
  "http 1.1.0",
  "http-body-util",
@@ -2698,6 +2699,12 @@ version = "3.2.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "a0474425d51df81997e2f90a21591180b38eccf27292d755f3e30750225c175b"
 
+[[package]]
+name = "error_reporter"
+version = "1.0.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "31ae425815400e5ed474178a7a22e275a9687086a12ca63ec793ff292d8fdae8"
+
 [[package]]
 name = "escape8259"
 version = "0.5.2"

cli/http_util.rs

@@ -390,10 +390,10 @@ impl HttpClient {
     let response = match self.client.clone().send(request).await {
       Ok(resp) => resp,
       Err(err) => {
-        if is_error_connect(&err) {
+        if err.is_connect_error() {
           return Ok(FetchOnceResult::RequestError(err.to_string()));
         }
-        return Err(err);
+        return Err(err.into());
       }
     };
 
@@ -531,7 +531,7 @@ impl HttpClient {
       .clone()
       .send(req)
       .await
-      .map_err(DownloadError::Fetch)?;
+      .map_err(|e| DownloadError::Fetch(e.into()))?;
     let status = response.status();
     if status.is_redirection() {
       for _ in 0..5 {
@@ -551,7 +551,7 @@ impl HttpClient {
           .clone()
           .send(req)
           .await
-          .map_err(DownloadError::Fetch)?;
+          .map_err(|e| DownloadError::Fetch(e.into()))?;
         let status = new_response.status();
         if status.is_redirection() {
           response = new_response;
@@ -567,13 +567,6 @@ impl HttpClient {
   }
 }
 
-fn is_error_connect(err: &AnyError) -> bool {
-  err
-    .downcast_ref::<hyper_util::client::legacy::Error>()
-    .map(|err| err.is_connect())
-    .unwrap_or(false)
-}
-
 async fn get_response_body_with_progress(
   response: http::Response<deno_fetch::ResBody>,
   progress_guard: Option<&UpdateGuard>,
@@ -685,7 +678,7 @@ impl RequestBuilder {
   pub async fn send(
     self,
   ) -> Result<http::Response<deno_fetch::ResBody>, AnyError> {
-    self.client.send(self.req).await
+    self.client.send(self.req).await.map_err(Into::into)
   }
 
   pub fn build(self) -> http::Request<deno_fetch::ReqBody> {

ext/fetch/26_fetch.js

@@ -65,7 +65,7 @@ const REQUEST_BODY_HEADER_NAMES = [
 
 /**
  * @param {number} rid
- * @returns {Promise<{ status: number, statusText: string, headers: [string, string][], url: string, responseRid: number, error: string? }>}
+ * @returns {Promise<{ status: number, statusText: string, headers: [string, string][], url: string, responseRid: number, error: [string, string]? }>}
  */
 function opFetchSend(rid) {
   return op_fetch_send(rid);
@@ -177,8 +177,9 @@ async function mainFetch(req, recursive, terminator) {
     }
   }
   // Re-throw any body errors
-  if (resp.error) {
-    throw new TypeError("body failed", { cause: new Error(resp.error) });
+  if (resp.error !== null) {
+    const { 0: message, 1: cause } = resp.error;
+    throw new TypeError(message, { cause: new Error(cause) });
   }
   if (terminator.aborted) return abortedNetworkError();
 

ext/fetch/Cargo.toml

@@ -21,6 +21,7 @@ deno_core.workspace = true
 deno_permissions.workspace = true
 deno_tls.workspace = true
 dyn-clone = "1"
+error_reporter = "1"
 http.workspace = true
 http-body-util.workspace = true
 hyper.workspace = true

ext/fetch/lib.rs

@@ -26,6 +26,7 @@ use deno_core::futures::Future;
 use deno_core::futures::FutureExt;
 use deno_core::futures::Stream;
 use deno_core::futures::StreamExt;
+use deno_core::futures::TryFutureExt;
 use deno_core::op2;
 use deno_core::unsync::spawn;
 use deno_core::url::Url;
@@ -429,7 +430,7 @@ where
 
       let mut request = http::Request::new(body);
       *request.method_mut() = method.clone();
-      *request.uri_mut() = uri;
+      *request.uri_mut() = uri.clone();
 
       if let Some((username, password)) = maybe_authority {
         request.headers_mut().insert(
@@ -469,8 +470,15 @@ where
       let cancel_handle = CancelHandle::new_rc();
       let cancel_handle_ = cancel_handle.clone();
 
-      let fut =
-        async move { client.send(request).or_cancel(cancel_handle_).await };
+      let fut = {
+        async move {
+          client
+            .send(request)
+            .map_err(Into::into)
+            .or_cancel(cancel_handle_)
+            .await
+        }
+      };
 
       let request_rid = state.resource_table.add(FetchRequestResource {
         future: Box::pin(fut),
@@ -532,7 +540,11 @@ pub struct FetchResponse {
   pub content_length: Option<u64>,
   pub remote_addr_ip: Option<String>,
   pub remote_addr_port: Option<u16>,
-  pub error: Option<String>,
+  /// This field is populated if some error occurred which needs to be
+  /// reconstructed in the JS side to set the error _cause_.
+  /// In the tuple, the first element is an error message and the second one is
+  /// an error cause.
+  pub error: Option<(String, String)>,
 }
 
 #[op2(async)]
@@ -558,16 +570,16 @@ pub async fn op_fetch_send(
       // reconstruct an error chain (eg: `new TypeError(..., { cause: new Error(...) })`).
       // TODO(mmastrac): it would be a lot easier if we just passed a v8::Global through here instead
       let mut err_ref: &dyn std::error::Error = err.as_ref();
-      while let Some(err) = std::error::Error::source(err_ref) {
-        if let Some(err) = err.downcast_ref::<hyper::Error>() {
-          if let Some(err) = std::error::Error::source(err) {
+      while let Some(err_src) = std::error::Error::source(err_ref) {
+        if let Some(err_src) = err_src.downcast_ref::<hyper::Error>() {
+          if let Some(err_src) = std::error::Error::source(err_src) {
             return Ok(FetchResponse {
-              error: Some(err.to_string()),
+              error: Some((err.to_string(), err_src.to_string())),
               ..Default::default()
             });
           }
         }
-        err_ref = err;
+        err_ref = err_src;
       }
 
       return Err(type_error(err.to_string()));
@@ -1082,11 +1094,41 @@ type Connector = proxy::ProxyConnector<HttpConnector>;
 #[allow(clippy::declare_interior_mutable_const)]
 const STAR_STAR: HeaderValue = HeaderValue::from_static("*/*");
 
+#[derive(Debug)]
+pub struct ClientSendError {
+  uri: Uri,
+  source: hyper_util::client::legacy::Error,
+}
+
+impl ClientSendError {
+  pub fn is_connect_error(&self) -> bool {
+    self.source.is_connect()
+  }
+}
+
+impl std::fmt::Display for ClientSendError {
+  fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+    write!(
+      f,
+      "error sending request for url ({uri}): {source}",
+      uri = self.uri,
+      // NOTE: we can use `std::error::Report` instead once it's stabilized.
+      source = error_reporter::Report::new(&self.source),
+    )
+  }
+}
+
+impl std::error::Error for ClientSendError {
+  fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+    Some(&self.source)
+  }
+}
+
 impl Client {
   pub async fn send(
     self,
     mut req: http::Request<ReqBody>,
-  ) -> Result<http::Response<ResBody>, AnyError> {
+  ) -> Result<http::Response<ResBody>, ClientSendError> {
     req
       .headers_mut()
       .entry(USER_AGENT)
@@ -1098,7 +1140,13 @@ impl Client {
       req.headers_mut().insert(PROXY_AUTHORIZATION, auth.clone());
     }
 
-    let resp = self.inner.oneshot(req).await?;
+    let uri = req.uri().clone();
+
+    let resp = self
+      .inner
+      .oneshot(req)
+      .await
+      .map_err(|e| ClientSendError { uri, source: e })?;
     Ok(resp.map(|b| b.map_err(|e| anyhow!(e)).boxed()))
   }
 }

tests/specs/cert/localhost_unsafe_ssl/localhost_unsafe_ssl.ts.out

@@ -1,3 +1,3 @@
 DANGER: TLS certificate validation is disabled for: deno.land
-error: Import 'https://localhost:5545/subdir/mod2.ts' failed: client error[WILDCARD]
+error: Import 'https://localhost:5545/subdir/mod2.ts' failed: error sending request for url (https://localhost:5545/subdir/mod2.ts): client error[WILDCARD]
     at file:///[WILDCARD]/cafile_url_imports.ts:[WILDCARD]

tests/testdata/run/fetch_async_error_stack.ts.out

@@ -1,4 +1,4 @@
-error: Uncaught (in promise) TypeError: client error[WILDCARD]
+error: Uncaught (in promise) TypeError: error sending request for url (https://nonexistent.deno.land/): client error[WILDCARD]
 await fetch("https://nonexistent.deno.land/");
 ^[WILDCARD]
     at async fetch (ext:[WILDCARD])

tests/unit/fetch_test.ts

@@ -1976,14 +1976,17 @@ Deno.test(
       },
     });
 
-    const err = await assertRejects(() =>
-      fetch(`http://localhost:${listenPort}/`, {
-        body: stream,
-        method: "POST",
-      })
+    const url = `http://localhost:${listenPort}/`;
+    const err = await assertRejects(
+      () =>
+        fetch(url, {
+          body: stream,
+          method: "POST",
+        }),
+      TypeError,
+      `error sending request for url (${url}): client error (SendRequest): error from user's Body stream`,
     );
 
-    assert(err instanceof TypeError, `err was not a TypeError ${err}`);
     assert(err.cause, `err.cause was null ${err}`);
     assert(
       err.cause instanceof Error,
@@ -2060,3 +2063,14 @@ Deno.test("URL authority is used as 'Authorization' header", async () => {
   await server.finished;
   assertEquals(authHeader, "Basic ZGVubzpsYW5k");
 });
+
+Deno.test(
+  { permissions: { net: true } },
+  async function errorMessageIncludesUrlAndDetails() {
+    await assertRejects(
+      () => fetch("http://example.invalid"),
+      TypeError,
+      "error sending request for url (http://example.invalid/): client error (Connect): dns error: ",
+    );
+  },
+);