Skip to content

Commit 355d899

Browse files
authored
fix: fetch fd leak (#31375)
Solution for: #31080 Modern reqwest (0.12+) can safely share clients across tokio runtimes. Because it uses the current runtime's executor instead of spawning background threads. Using a single shared client prevents resource leaks when worker threads terminate. ## Context It seems that we started to use one client per thread because hyper and reqwest was not safe to share across tokio runtimes (#23699). But seems it's fixed at latest reqwest versions (seanmonstar/reqwest#1148 (comment)). This PR is using one client again, as we now already use the version of reqwest with this fix.
1 parent 2b20d4a commit 355d899

File tree

2 files changed

+18
-42
lines changed

2 files changed

+18
-42
lines changed

cli/http_util.rs

Lines changed: 17 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,11 @@
11
// Copyright 2018-2025 the Deno authors. MIT license.
22

3-
use std::collections::HashMap;
43
use std::sync::Arc;
5-
use std::thread::ThreadId;
64

75
use boxed_error::Boxed;
86
use deno_cache_dir::file_fetcher::RedirectHeaderParseError;
97
use deno_core::error::AnyError;
108
use deno_core::futures::StreamExt;
11-
use deno_core::parking_lot::Mutex;
129
use deno_core::serde;
1310
use deno_core::serde_json;
1411
use deno_core::url::Url;
@@ -26,6 +23,7 @@ use http::header::CONTENT_LENGTH;
2623
use http::header::HeaderName;
2724
use http::header::HeaderValue;
2825
use http_body_util::BodyExt;
26+
use once_cell::sync::OnceCell;
2927
use thiserror::Error;
3028

3129
use crate::util::progress_bar::UpdateGuard;
@@ -41,10 +39,7 @@ pub enum SendError {
4139
pub struct HttpClientProvider {
4240
options: CreateHttpClientOptions,
4341
root_cert_store_provider: Option<Arc<dyn RootCertStoreProvider>>,
44-
// it's not safe to share a reqwest::Client across tokio runtimes,
45-
// so we store these Clients keyed by thread id
46-
// https://github.com/seanmonstar/reqwest/issues/1148#issuecomment-910868788
47-
clients_by_thread_id: Mutex<HashMap<ThreadId, deno_fetch::Client>>,
42+
client: OnceCell<deno_fetch::Client>,
4843
}
4944

5045
impl std::fmt::Debug for HttpClientProvider {
@@ -66,33 +61,25 @@ impl HttpClientProvider {
6661
..Default::default()
6762
},
6863
root_cert_store_provider,
69-
clients_by_thread_id: Default::default(),
64+
client: OnceCell::new(),
7065
}
7166
}
7267

7368
pub fn get_or_create(&self) -> Result<HttpClient, JsErrorBox> {
74-
use std::collections::hash_map::Entry;
75-
let thread_id = std::thread::current().id();
76-
let mut clients = self.clients_by_thread_id.lock();
77-
let entry = clients.entry(thread_id);
78-
match entry {
79-
Entry::Occupied(entry) => Ok(HttpClient::new(entry.get().clone())),
80-
Entry::Vacant(entry) => {
81-
let client = create_http_client(
82-
DENO_VERSION_INFO.user_agent,
83-
CreateHttpClientOptions {
84-
root_cert_store: match &self.root_cert_store_provider {
85-
Some(provider) => Some(provider.get_or_try_init()?.clone()),
86-
None => None,
87-
},
88-
..self.options.clone()
69+
let client = self.client.get_or_try_init(|| {
70+
create_http_client(
71+
DENO_VERSION_INFO.user_agent,
72+
CreateHttpClientOptions {
73+
root_cert_store: match &self.root_cert_store_provider {
74+
Some(provider) => Some(provider.get_or_try_init()?.clone()),
75+
None => None,
8976
},
90-
)
91-
.map_err(JsErrorBox::from_err)?;
92-
entry.insert(client.clone());
93-
Ok(HttpClient::new(client))
94-
}
95-
}
77+
..self.options.clone()
78+
},
79+
)
80+
.map_err(JsErrorBox::from_err)
81+
})?;
82+
Ok(HttpClient::new(client.clone()))
9683
}
9784
}
9885

@@ -179,21 +166,13 @@ impl HttpClientResponse {
179166
#[derive(Debug)]
180167
pub struct HttpClient {
181168
client: deno_fetch::Client,
182-
// don't allow sending this across threads because then
183-
// it might be shared accidentally across tokio runtimes
184-
// which will cause issues
185-
// https://github.com/seanmonstar/reqwest/issues/1148#issuecomment-910868788
186-
_unsend_marker: deno_core::unsync::UnsendMarker,
187169
}
188170

189171
impl HttpClient {
190172
// DO NOT make this public. You should always be creating one of these from
191173
// the HttpClientProvider
192174
fn new(client: deno_fetch::Client) -> Self {
193-
Self {
194-
client,
195-
_unsend_marker: deno_core::unsync::UnsendMarker::default(),
196-
}
175+
Self { client }
197176
}
198177

199178
pub fn get(&self, url: Url) -> Result<RequestBuilder, http::Error> {

runtime/ops/web_worker/sync_fetch.rs

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -93,10 +93,7 @@ pub fn op_worker_sync_fetch(
9393
let handle = state.borrow::<WebWorkerInternalHandle>().clone();
9494
assert_eq!(handle.worker_type, WorkerThreadType::Classic);
9595

96-
// it's not safe to share a client across tokio runtimes, so create a fresh one
97-
// https://github.com/seanmonstar/reqwest/issues/1148#issuecomment-910868788
98-
let options = state.borrow::<deno_fetch::Options>().clone();
99-
let client = deno_fetch::create_client_from_options(&options)
96+
let client = deno_fetch::get_or_create_client_from_state(state)
10097
.map_err(FetchError::ClientCreate)?;
10198

10299
// TODO(andreubotella) It's not good to throw an exception related to blob

0 commit comments

Comments
 (0)