Turbopack: Make turbo-tasks-fetch a bit more OOP-like (vercel#81995)

This was a request from @lukesandberg to make this API a bit more object-oriented: vercel#81818 (comment)

- `ReqwestClientConfig` is now just `FetchClient`.
- `fetch` is a method on `FetchClient` instead of a function taking a config.
- **Bonus change:** `lib.rs` is now a tiny stub that just re-exports things from other modules (`client.rs`, `error.rs`, `response.rs`).

Author: bgw

crates/next-core/src/next_config.rs

@@ -9,7 +9,7 @@ use turbo_tasks::{
     trace::TraceRawVcs,
 };
 use turbo_tasks_env::{EnvMap, ProcessEnv};
-use turbo_tasks_fetch::ReqwestClientConfig;
+use turbo_tasks_fetch::FetchClient;
 use turbo_tasks_fs::FileSystemPath;
 use turbopack::module_options::{
     ConditionItem, ConditionPath, LoaderRuleItem, OptionWebpackRules,
@@ -1725,10 +1725,7 @@ impl NextConfig {
     }
 
     #[turbo_tasks::function]
-    pub async fn reqwest_client_config(
-        &self,
-        env: Vc<Box<dyn ProcessEnv>>,
-    ) -> Result<Vc<ReqwestClientConfig>> {
+    pub async fn fetch_client(&self, env: Vc<Box<dyn ProcessEnv>>) -> Result<Vc<FetchClient>> {
         // Support both an env var and the experimental flag to provide more flexibility to
         // developers on locked down systems, depending on if they want to configure this on a
         // per-system or per-project basis.
@@ -1742,7 +1739,7 @@ impl NextConfig {
             })
             .or(self.experimental.turbopack_use_system_tls_certs)
             .unwrap_or(false);
-        Ok(ReqwestClientConfig {
+        Ok(FetchClient {
             tls_built_in_webpki_certs: !use_system_tls_certs,
             tls_built_in_native_certs: use_system_tls_certs,
         }

crates/next-core/src/next_font/google/mod.rs

@@ -9,7 +9,7 @@ use turbo_rcstr::{RcStr, rcstr};
 use turbo_tasks::{Completion, FxIndexMap, ResolvedVc, Vc};
 use turbo_tasks_bytes::stream::SingleValue;
 use turbo_tasks_env::{CommandLineProcessEnv, ProcessEnv};
-use turbo_tasks_fetch::{HttpResponseBody, ReqwestClientConfig, fetch};
+use turbo_tasks_fetch::{FetchClient, HttpResponseBody};
 use turbo_tasks_fs::{
     DiskFileSystem, File, FileContent, FileSystem, FileSystemPath,
     json::parse_json_with_source_context,
@@ -182,7 +182,7 @@ pub struct NextFontGoogleCssModuleReplacer {
     project_path: FileSystemPath,
     execution_context: ResolvedVc<ExecutionContext>,
     next_mode: ResolvedVc<NextMode>,
-    reqwest_client_config: ResolvedVc<ReqwestClientConfig>,
+    fetch_client: ResolvedVc<FetchClient>,
 }
 
 #[turbo_tasks::value_impl]
@@ -192,13 +192,13 @@ impl NextFontGoogleCssModuleReplacer {
         project_path: FileSystemPath,
         execution_context: ResolvedVc<ExecutionContext>,
         next_mode: ResolvedVc<NextMode>,
-        reqwest_client_config: ResolvedVc<ReqwestClientConfig>,
+        fetch_client: ResolvedVc<FetchClient>,
     ) -> Vc<Self> {
         Self::cell(NextFontGoogleCssModuleReplacer {
             project_path,
             execution_context,
             next_mode,
-            reqwest_client_config,
+            fetch_client,
         })
     }
 
@@ -233,9 +233,9 @@ impl NextFontGoogleCssModuleReplacer {
             .map_or_else(
                 || {
                     fetch_real_stylesheet(
+                        *self.fetch_client,
                         stylesheet_url.clone(),
                         css_virtual_path.clone(),
-                        *self.reqwest_client_config,
                     )
                     .boxed()
                 },
@@ -375,19 +375,16 @@ struct NextFontGoogleFontFileOptions {
 #[turbo_tasks::value(shared)]
 pub struct NextFontGoogleFontFileReplacer {
     project_path: FileSystemPath,
-    reqwest_client_config: ResolvedVc<ReqwestClientConfig>,
+    fetch_client: ResolvedVc<FetchClient>,
 }
 
 #[turbo_tasks::value_impl]
 impl NextFontGoogleFontFileReplacer {
     #[turbo_tasks::function]
-    pub fn new(
-        project_path: FileSystemPath,
-        reqwest_client_config: ResolvedVc<ReqwestClientConfig>,
-    ) -> Vc<Self> {
+    pub fn new(project_path: FileSystemPath, fetch_client: ResolvedVc<FetchClient>) -> Vc<Self> {
         Self::cell(NextFontGoogleFontFileReplacer {
             project_path,
-            reqwest_client_config,
+            fetch_client,
         })
     }
 }
@@ -443,12 +440,9 @@ impl ImportMappingReplacement for NextFontGoogleFontFileReplacer {
 
         // doesn't seem ideal to download the font into a string, but probably doesn't
         // really matter either.
-        let Some(font) = fetch_from_google_fonts(
-            url.into(),
-            font_virtual_path.clone(),
-            *self.reqwest_client_config,
-        )
-        .await?
+        let Some(font) =
+            fetch_from_google_fonts(*self.fetch_client, url.into(), font_virtual_path.clone())
+                .await?
         else {
             return Ok(ImportMapResult::Result(ResolveResult::unresolvable()).cell());
         };
@@ -670,27 +664,23 @@ fn font_file_options_from_query_map(query: &RcStr) -> Result<NextFontGoogleFontF
 }
 
 async fn fetch_real_stylesheet(
+    fetch_client: Vc<FetchClient>,
     stylesheet_url: RcStr,
     css_virtual_path: FileSystemPath,
-    reqwest_client_config: Vc<ReqwestClientConfig>,
 ) -> Result<Option<Vc<RcStr>>> {
-    let body =
-        fetch_from_google_fonts(stylesheet_url, css_virtual_path, reqwest_client_config).await?;
+    let body = fetch_from_google_fonts(fetch_client, stylesheet_url, css_virtual_path).await?;
 
     Ok(body.map(|body| body.to_string()))
 }
 
 async fn fetch_from_google_fonts(
+    fetch_client: Vc<FetchClient>,
     url: RcStr,
     virtual_path: FileSystemPath,
-    reqwest_client_config: Vc<ReqwestClientConfig>,
 ) -> Result<Option<Vc<HttpResponseBody>>> {
-    let result = fetch(
-        url,
-        Some(rcstr!(USER_AGENT_FOR_GOOGLE_FONTS)),
-        reqwest_client_config,
-    )
-    .await?;
+    let result = fetch_client
+        .fetch(url, Some(rcstr!(USER_AGENT_FOR_GOOGLE_FONTS)))
+        .await?;
 
     Ok(match *result {
         Ok(r) => Some(*r.await?.body),

crates/next-core/src/next_import_map.rs

@@ -1022,15 +1022,15 @@ async fn insert_next_shared_aliases(
         next_font_google_replacer_mapping,
     );
 
-    let reqwest_client_config = next_config.reqwest_client_config(execution_context.env());
+    let fetch_client = next_config.fetch_client(execution_context.env());
     import_map.insert_alias(
         AliasPattern::exact("@vercel/turbopack-next/internal/font/google/cssmodule.module.css"),
         ImportMapping::Dynamic(ResolvedVc::upcast(
             NextFontGoogleCssModuleReplacer::new(
                 project_path.clone(),
                 execution_context,
                 next_mode,
-                reqwest_client_config,
+                fetch_client,
             )
             .to_resolved()
             .await?,
@@ -1041,7 +1041,7 @@ async fn insert_next_shared_aliases(
     import_map.insert_alias(
         AliasPattern::exact(GOOGLE_FONTS_INTERNAL_PREFIX),
         ImportMapping::Dynamic(ResolvedVc::upcast(
-            NextFontGoogleFontFileReplacer::new(project_path.clone(), reqwest_client_config)
+            NextFontGoogleFontFileReplacer::new(project_path.clone(), fetch_client)
                 .to_resolved()
                 .await?,
         ))

turbopack/crates/turbo-tasks-fetch/Cargo.toml

@@ -20,7 +20,7 @@ workspace = true
 [target.'cfg(all(target_os = "windows", target_arch = "aarch64"))'.dependencies]
 reqwest = { workspace = true, features = ["native-tls"] }
 
-# If you modify this cfg, make sure you update `ReqwestClientConfig::try_build`.
+# If you modify this cfg, make sure you update `FetchClient::try_build_uncached_reqwest_client`.
 [target.'cfg(not(any(all(target_os = "windows", target_arch = "aarch64"), target_arch="wasm32")))'.dependencies]
 reqwest = { workspace = true, features = ["rustls-tls-webpki-roots", "rustls-tls-native-roots"] }
 

turbopack/crates/turbo-tasks-fetch/src/client.rs

@@ -0,0 +1,156 @@
+use std::{hash::Hash, sync::LazyLock};
+
+use anyhow::Result;
+use quick_cache::sync::Cache;
+use serde::{Deserialize, Serialize};
+use turbo_rcstr::RcStr;
+use turbo_tasks::{
+    NonLocalValue, ReadRef, Vc, duration_span, mark_session_dependent, trace::TraceRawVcs,
+};
+
+use crate::{FetchError, FetchResult, HttpResponse, HttpResponseBody};
+
+const MAX_CLIENTS: usize = 16;
+static CLIENT_CACHE: LazyLock<Cache<ReadRef<FetchClient>, reqwest::Client>> =
+    LazyLock::new(|| Cache::new(MAX_CLIENTS));
+
+#[derive(Hash, PartialEq, Eq, Serialize, Deserialize, NonLocalValue, Debug, TraceRawVcs)]
+pub enum ProxyConfig {
+    Http(RcStr),
+    Https(RcStr),
+}
+
+/// Represents the configuration needed to construct a [`reqwest::Client`].
+///
+/// This is used to cache clients keyed by their configuration, so the configuration should contain
+/// as few fields as possible and change infrequently.
+///
+/// This is needed because [`reqwest::ClientBuilder`] does not implement the required traits. This
+/// factory cannot be a closure because closures do not implement `Eq` or `Hash`.
+#[turbo_tasks::value(shared)]
+#[derive(Hash)]
+pub struct FetchClient {
+    /// Whether to load embedded webpki root certs with rustls. Default is true.
+    ///
+    /// Ignored for:
+    /// - Windows on ARM, which uses `native-tls` instead of `rustls-tls`.
+    /// - Ignored for WASM targets, which use the runtime's TLS implementation.
+    pub tls_built_in_webpki_certs: bool,
+    /// Whether to load native root certs using the `rustls-native-certs` crate. This may make
+    /// reqwest client initialization slower, so it's not used by default.
+    ///
+    /// Ignored for:
+    /// - Windows on ARM, which uses `native-tls` instead of `rustls-tls`.
+    /// - Ignored for WASM targets, which use the runtime's TLS implementation.
+    pub tls_built_in_native_certs: bool,
+}
+
+impl Default for FetchClient {
+    fn default() -> Self {
+        Self {
+            tls_built_in_webpki_certs: true,
+            tls_built_in_native_certs: false,
+        }
+    }
+}
+
+impl FetchClient {
+    /// Returns a cached instance of `reqwest::Client` it exists, otherwise constructs a new one.
+    ///
+    /// The cache is bound in size to prevent accidental blowups or leaks. However, in practice,
+    /// very few clients should be created, likely only when the bundler configuration changes.
+    ///
+    /// Client construction is largely deterministic, aside from changes to system TLS
+    /// configuration.
+    ///
+    /// The reqwest client fails to construct if the TLS backend cannot be initialized, or the
+    /// resolver cannot load the system configuration. These failures should be treated as
+    /// cached for some amount of time, but ultimately transient (e.g. using
+    /// [`turbo_tasks::mark_session_dependent`]).
+    pub fn try_get_cached_reqwest_client(
+        self: ReadRef<FetchClient>,
+    ) -> reqwest::Result<reqwest::Client> {
+        CLIENT_CACHE.get_or_insert_with(&self, {
+            let this = ReadRef::clone(&self);
+            move || this.try_build_uncached_reqwest_client()
+        })
+    }
+
+    fn try_build_uncached_reqwest_client(&self) -> reqwest::Result<reqwest::Client> {
+        let client_builder = reqwest::Client::builder();
+
+        // make sure this cfg matches the one in `Cargo.toml`!
+        #[cfg(not(any(
+            all(target_os = "windows", target_arch = "aarch64"),
+            target_arch = "wasm32"
+        )))]
+        let client_builder = client_builder
+            .tls_built_in_root_certs(false)
+            .tls_built_in_webpki_certs(self.tls_built_in_webpki_certs)
+            .tls_built_in_native_certs(self.tls_built_in_native_certs);
+
+        client_builder.build()
+    }
+}
+
+#[turbo_tasks::value_impl]
+impl FetchClient {
+    #[turbo_tasks::function(network)]
+    pub async fn fetch(
+        self: Vc<FetchClient>,
+        url: RcStr,
+        user_agent: Option<RcStr>,
+    ) -> Result<Vc<FetchResult>> {
+        let url_ref = &*url;
+        let this = self.await?;
+        let response_result: reqwest::Result<HttpResponse> = async move {
+            let reqwest_client = this.try_get_cached_reqwest_client()?;
+
+            let mut builder = reqwest_client.get(url_ref);
+            if let Some(user_agent) = user_agent {
+                builder = builder.header("User-Agent", user_agent.as_str());
+            }
+
+            let response = {
+                let _span = duration_span!("fetch request", url = url_ref);
+                builder.send().await
+            }
+            .and_then(|r| r.error_for_status())?;
+
+            let status = response.status().as_u16();
+
+            let body = {
+                let _span = duration_span!("fetch response", url = url_ref);
+                response.bytes().await?
+            }
+            .to_vec();
+
+            Ok(HttpResponse {
+                status,
+                body: HttpResponseBody(body).resolved_cell(),
+            })
+        }
+        .await;
+
+        match response_result {
+            Ok(resp) => Ok(Vc::cell(Ok(resp.resolved_cell()))),
+            Err(err) => {
+                // the client failed to construct or the HTTP request failed
+                mark_session_dependent();
+                Ok(Vc::cell(Err(
+                    FetchError::from_reqwest_error(&err, &url).resolved_cell()
+                )))
+            }
+        }
+    }
+}
+
+#[doc(hidden)]
+pub fn __test_only_reqwest_client_cache_clear() {
+    CLIENT_CACHE.clear()
+}
+
+#[doc(hidden)]
+pub fn __test_only_reqwest_client_cache_len() -> usize {
+    CLIENT_CACHE.len()
+}