Skip to content

Commit d527b63

Browse files
dsherretdsherret
authored
fix(workers): importScripts concurrently and use a new reqwest::Client per importScripts (#23699)
1. We were polling each future in sequence, so this meant it was fetching scripts in sequence. 2. It's not safe to share `reqwest::Client` across tokio runtimes (seanmonstar/reqwest#1148 (comment))
1 parent 4b0f22e commit d527b63

File tree

2 files changed

+118
-107
lines changed

2 files changed

+118
-107
lines changed

‎ext/fetch/lib.rs

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -187,27 +187,33 @@ pub fn get_or_create_client_from_state(
187187
Ok(client.clone())
188188
} else {
189189
let options = state.borrow::<Options>();
190-
let client = create_http_client(
191-
&options.user_agent,
192-
CreateHttpClientOptions {
193-
root_cert_store: options.root_cert_store()?,
194-
ca_certs: vec![],
195-
proxy: options.proxy.clone(),
196-
unsafely_ignore_certificate_errors: options
197-
.unsafely_ignore_certificate_errors
198-
.clone(),
199-
client_cert_chain_and_key: options.client_cert_chain_and_key.clone(),
200-
pool_max_idle_per_host: None,
201-
pool_idle_timeout: None,
202-
http1: true,
203-
http2: true,
204-
},
205-
)?;
190+
let client = create_client_from_options(options)?;
206191
state.put::<reqwest::Client>(client.clone());
207192
Ok(client)
208193
}
209194
}
210195

196+
pub fn create_client_from_options(
197+
options: &Options,
198+
) -> Result<reqwest::Client, AnyError> {
199+
create_http_client(
200+
&options.user_agent,
201+
CreateHttpClientOptions {
202+
root_cert_store: options.root_cert_store()?,
203+
ca_certs: vec![],
204+
proxy: options.proxy.clone(),
205+
unsafely_ignore_certificate_errors: options
206+
.unsafely_ignore_certificate_errors
207+
.clone(),
208+
client_cert_chain_and_key: options.client_cert_chain_and_key.clone(),
209+
pool_max_idle_per_host: None,
210+
pool_idle_timeout: None,
211+
http1: true,
212+
http2: true,
213+
},
214+
)
215+
}
216+
211217
#[allow(clippy::type_complexity)]
212218
pub struct ResourceToBodyAdapter(
213219
Rc<dyn Resource>,

‎runtime/ops/web_worker/sync_fetch.rs

Lines changed: 96 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ use crate::web_worker::WebWorkerInternalHandle;
66
use crate::web_worker::WebWorkerType;
77
use deno_core::error::type_error;
88
use deno_core::error::AnyError;
9+
use deno_core::futures::StreamExt;
910
use deno_core::op2;
1011
use deno_core::url::Url;
1112
use deno_core::OpState;
@@ -15,7 +16,6 @@ use deno_websocket::DomExceptionNetworkError;
1516
use hyper::body::Bytes;
1617
use serde::Deserialize;
1718
use serde::Serialize;
18-
use tokio::task::JoinHandle;
1919

2020
// TODO(andreubotella) Properly parse the MIME type
2121
fn mime_type_essence(mime_type: &str) -> String {
@@ -38,12 +38,15 @@ pub struct SyncFetchScript {
3838
pub fn op_worker_sync_fetch(
3939
state: &mut OpState,
4040
#[serde] scripts: Vec<String>,
41-
mut loose_mime_checks: bool,
41+
loose_mime_checks: bool,
4242
) -> Result<Vec<SyncFetchScript>, AnyError> {
4343
let handle = state.borrow::<WebWorkerInternalHandle>().clone();
4444
assert_eq!(handle.worker_type, WebWorkerType::Classic);
4545

46-
let client = deno_fetch::get_or_create_client_from_state(state)?;
46+
// it's not safe to share a client across tokio runtimes, so create a fresh one
47+
// https://github.com/seanmonstar/reqwest/issues/1148#issuecomment-910868788
48+
let options = state.borrow::<deno_fetch::Options>().clone();
49+
let client = deno_fetch::create_client_from_options(&options)?;
4750

4851
// TODO(andreubotella) It's not good to throw an exception related to blob
4952
// URLs when none of the script URLs use the blob scheme.
@@ -62,107 +65,109 @@ pub fn op_worker_sync_fetch(
6265
.enable_time()
6366
.build()?;
6467

65-
let handles: Vec<_> = scripts
66-
.into_iter()
67-
.map(|script| -> JoinHandle<Result<SyncFetchScript, AnyError>> {
68-
let client = client.clone();
69-
let blob_store = blob_store.clone();
70-
runtime.spawn(async move {
71-
let script_url = Url::parse(&script)
72-
.map_err(|_| type_error("Invalid script URL"))?;
73-
74-
let (body, mime_type, res_url) = match script_url.scheme() {
75-
"http" | "https" => {
76-
let resp =
77-
client.get(script_url).send().await?.error_for_status()?;
78-
79-
let res_url = resp.url().to_string();
80-
81-
// TODO(andreubotella) Properly run fetch's "extract a MIME type".
82-
let mime_type = resp
83-
.headers()
84-
.get("Content-Type")
85-
.and_then(|v| v.to_str().ok())
86-
.map(mime_type_essence);
87-
88-
// Always check the MIME type with HTTP(S).
89-
loose_mime_checks = false;
90-
91-
let body = resp.bytes().await?;
92-
93-
(body, mime_type, res_url)
94-
}
95-
"data" => {
96-
let data_url = DataUrl::process(&script)
97-
.map_err(|e| type_error(format!("{e:?}")))?;
68+
runtime.block_on(async move {
69+
let mut futures = scripts
70+
.into_iter()
71+
.map(|script| {
72+
let client = client.clone();
73+
let blob_store = blob_store.clone();
74+
deno_core::unsync::spawn(async move {
75+
let script_url = Url::parse(&script)
76+
.map_err(|_| type_error("Invalid script URL"))?;
77+
let mut loose_mime_checks = loose_mime_checks;
78+
79+
let (body, mime_type, res_url) = match script_url.scheme() {
80+
"http" | "https" => {
81+
let resp =
82+
client.get(script_url).send().await?.error_for_status()?;
83+
84+
let res_url = resp.url().to_string();
85+
86+
// TODO(andreubotella) Properly run fetch's "extract a MIME type".
87+
let mime_type = resp
88+
.headers()
89+
.get("Content-Type")
90+
.and_then(|v| v.to_str().ok())
91+
.map(mime_type_essence);
92+
93+
// Always check the MIME type with HTTP(S).
94+
loose_mime_checks = false;
95+
96+
let body = resp.bytes().await?;
97+
98+
(body, mime_type, res_url)
99+
}
100+
"data" => {
101+
let data_url = DataUrl::process(&script)
102+
.map_err(|e| type_error(format!("{e:?}")))?;
98103

99-
let mime_type = {
100-
let mime = data_url.mime_type();
101-
format!("{}/{}", mime.type_, mime.subtype)
102-
};
104+
let mime_type = {
105+
let mime = data_url.mime_type();
106+
format!("{}/{}", mime.type_, mime.subtype)
107+
};
103108

104-
let (body, _) = data_url
105-
.decode_to_vec()
106-
.map_err(|e| type_error(format!("{e:?}")))?;
109+
let (body, _) = data_url
110+
.decode_to_vec()
111+
.map_err(|e| type_error(format!("{e:?}")))?;
107112

108-
(Bytes::from(body), Some(mime_type), script)
109-
}
110-
"blob" => {
111-
let blob =
112-
blob_store.get_object_url(script_url).ok_or_else(|| {
113-
type_error("Blob for the given URL not found.")
114-
})?;
113+
(Bytes::from(body), Some(mime_type), script)
114+
}
115+
"blob" => {
116+
let blob =
117+
blob_store.get_object_url(script_url).ok_or_else(|| {
118+
type_error("Blob for the given URL not found.")
119+
})?;
115120

116-
let mime_type = mime_type_essence(&blob.media_type);
121+
let mime_type = mime_type_essence(&blob.media_type);
117122

118-
let body = blob.read_all().await?;
123+
let body = blob.read_all().await?;
119124

120-
(Bytes::from(body), Some(mime_type), script)
121-
}
122-
_ => {
123-
return Err(type_error(format!(
124-
"Classic scripts with scheme {}: are not supported in workers.",
125-
script_url.scheme()
126-
)))
127-
}
128-
};
129-
130-
if !loose_mime_checks {
131-
// TODO(andreubotella) Check properly for a Javascript MIME type.
132-
match mime_type.as_deref() {
133-
Some("application/javascript" | "text/javascript") => {}
134-
Some(mime_type) => {
135-
return Err(
136-
DomExceptionNetworkError {
137-
msg: format!("Invalid MIME type {mime_type:?}."),
138-
}
139-
.into(),
140-
)
125+
(Bytes::from(body), Some(mime_type), script)
126+
}
127+
_ => {
128+
return Err(type_error(format!(
129+
"Classic scripts with scheme {}: are not supported in workers.",
130+
script_url.scheme()
131+
)))
141132
}
142-
None => {
143-
return Err(
144-
DomExceptionNetworkError::new("Missing MIME type.").into(),
145-
)
133+
};
134+
135+
if !loose_mime_checks {
136+
// TODO(andreubotella) Check properly for a Javascript MIME type.
137+
match mime_type.as_deref() {
138+
Some("application/javascript" | "text/javascript") => {}
139+
Some(mime_type) => {
140+
return Err(
141+
DomExceptionNetworkError {
142+
msg: format!("Invalid MIME type {mime_type:?}."),
143+
}
144+
.into(),
145+
)
146+
}
147+
None => {
148+
return Err(
149+
DomExceptionNetworkError::new("Missing MIME type.").into(),
150+
)
151+
}
146152
}
147153
}
148-
}
149154

150-
let (text, _) = encoding_rs::UTF_8.decode_with_bom_removal(&body);
155+
let (text, _) = encoding_rs::UTF_8.decode_with_bom_removal(&body);
151156

152-
Ok(SyncFetchScript {
153-
url: res_url,
154-
script: text.into_owned(),
157+
Ok(SyncFetchScript {
158+
url: res_url,
159+
script: text.into_owned(),
160+
})
155161
})
156162
})
157-
})
158-
.collect();
159-
160-
let mut ret = Vec::with_capacity(handles.len());
161-
for handle in handles {
162-
let script = runtime.block_on(handle)??;
163-
ret.push(script);
164-
}
165-
Ok(ret)
163+
.collect::<deno_core::futures::stream::FuturesUnordered<_>>();
164+
let mut ret = Vec::with_capacity(futures.len());
165+
while let Some(result) = futures.next().await {
166+
let script = result??;
167+
ret.push(script);
168+
}
169+
Ok(ret)
170+
})
166171
});
167172

168173
thread.join().unwrap()

0 commit comments

Comments
 (0)