From e36b1a3aa88b31435b18a33448fc75eeb6dc8017 Mon Sep 17 00:00:00 2001 From: Yusuke Tanaka Date: Fri, 9 Aug 2024 00:47:15 +0900 Subject: [PATCH] fix(ext/fetch): include TCP src/dst socket info in error messages (#24939) This commit makes `fetch` error messages include source and destination TCP socket info i.e. port number and IP address for better debuggability. Closes #24922 --- Cargo.lock | 4 +-- Cargo.toml | 2 +- ext/fetch/lib.rs | 38 +++++++++++++++++++++----- tests/unit/fetch_test.ts | 59 ++++++++++++++++++++++++++++++++++------ 4 files changed, 84 insertions(+), 19 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index aa989b210f..238bfb89e5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3630,9 +3630,9 @@ dependencies = [ [[package]] name = "hyper-util" -version = "0.1.6" +version = "0.1.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3ab92f4f49ee4fb4f997c784b7a2e0fa70050211e0b6a287f898c3c9785ca956" +checksum = "cde7055719c54e36e95e8719f95883f22072a48ede39db7fc17a4e1d5281e9b9" dependencies = [ "bytes", "futures-channel", diff --git a/Cargo.toml b/Cargo.toml index cbf9940ca2..076ebdf4f6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -124,7 +124,7 @@ http_v02 = { package = "http", version = "0.2.9" } httparse = "1.8.0" hyper = { version = "1.4.1", features = ["full"] } hyper-rustls = { version = "0.27.2", default-features = false, features = ["http1", "http2", "tls12", "ring"] } -hyper-util = { version = "=0.1.6", features = ["tokio", "client", "client-legacy", "server", "server-auto"] } +hyper-util = { version = "=0.1.7", features = ["tokio", "client", "client-legacy", "server", "server-auto"] } hyper_v014 = { package = "hyper", version = "0.14.26", features = ["runtime", "http1"] } indexmap = { version = "2", features = ["serde"] } ipnet = "2.3" diff --git a/ext/fetch/lib.rs b/ext/fetch/lib.rs index 798acad0b7..fa85824f44 100644 --- a/ext/fetch/lib.rs +++ b/ext/fetch/lib.rs @@ -62,11 +62,13 @@ use http::header::HOST; use http::header::PROXY_AUTHORIZATION; use http::header::RANGE; use http::header::USER_AGENT; +use http::Extensions; use http::Method; use http::Uri; use http_body_util::BodyExt; use hyper::body::Frame; use hyper_util::client::legacy::connect::HttpConnector; +use hyper_util::client::legacy::connect::HttpInfo; use hyper_util::rt::TokioExecutor; use hyper_util::rt::TokioIo; use hyper_util::rt::TokioTimer; @@ -1104,17 +1106,39 @@ impl ClientSendError { pub fn is_connect_error(&self) -> bool { self.source.is_connect() } + + fn http_info(&self) -> Option { + let mut exts = Extensions::new(); + self.source.connect_info()?.get_extras(&mut exts); + exts.remove::() + } } 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), - ) + // NOTE: we can use `std::error::Report` instead once it's stabilized. + let detail = error_reporter::Report::new(&self.source); + + match self.http_info() { + Some(http_info) => { + write!( + f, + "error sending request from {src} for {uri} ({dst}): {detail}", + src = http_info.local_addr(), + uri = self.uri, + dst = http_info.remote_addr(), + detail = detail, + ) + } + None => { + write!( + f, + "error sending request for url ({uri}): {detail}", + uri = self.uri, + detail = detail, + ) + } + } } } diff --git a/tests/unit/fetch_test.ts b/tests/unit/fetch_test.ts index 5ebc0c86f8..9b2463bcc3 100644 --- a/tests/unit/fetch_test.ts +++ b/tests/unit/fetch_test.ts @@ -3,6 +3,7 @@ import { assert, assertEquals, assertRejects, + assertStringIncludes, assertThrows, delay, fail, @@ -1977,14 +1978,24 @@ Deno.test( }); 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`, + const err = await assertRejects(() => + fetch(url, { + body: stream, + method: "POST", + }) + ); + + assert(err instanceof TypeError, `err was ${err}`); + + assertStringIncludes( + err.message, + "error sending request from 127.0.0.1:", + `err.message was ${err.message}`, + ); + assertStringIncludes( + err.message, + ` for http://localhost:${listenPort}/ (127.0.0.1:${listenPort}): client error (SendRequest): error from user's Body stream`, + `err.message was ${err.message}`, ); assert(err.cause, `err.cause was null ${err}`); @@ -2066,7 +2077,7 @@ Deno.test("URL authority is used as 'Authorization' header", async () => { Deno.test( { permissions: { net: true } }, - async function errorMessageIncludesUrlAndDetails() { + async function errorMessageIncludesUrlAndDetailsWithNoTcpInfo() { await assertRejects( () => fetch("http://example.invalid"), TypeError, @@ -2074,3 +2085,33 @@ Deno.test( ); }, ); + +Deno.test( + { permissions: { net: true } }, + async function errorMessageIncludesUrlAndDetailsWithTcpInfo() { + const listener = Deno.listen({ port: listenPort }); + const server = (async () => { + const conn = await listener.accept(); + listener.close(); + // Immediately close the connection to simulate a connection error + conn.close(); + })(); + + const url = `http://localhost:${listenPort}`; + const err = await assertRejects(() => fetch(url)); + + assert(err instanceof TypeError, `${err}`); + assertStringIncludes( + err.message, + "error sending request from 127.0.0.1:", + `${err.message}`, + ); + assertStringIncludes( + err.message, + ` for http://localhost:${listenPort}/ (127.0.0.1:${listenPort}): client error (SendRequest): `, + `${err.message}`, + ); + + await server; + }, +);