fix(download_timeout): introduce RUSTUP_DOWNLOAD_TIMEOUT for overriding download timeout

As some (corporate) firewalls take too much time to verify the
downloads, the timeout is reached and the user cannot complete its
installation.

Besides changing the timeout from 30 secs to 3 mins, a new environment
variable is introduced to allow users to override the timeout value to
their taste.

Author: FranciscoTGouveia

doc/user-guide/src/environment-variables.md

@@ -66,6 +66,9 @@
 
 - `RUSTUP_TERM_WIDTH` (default: none). Allows to override the terminal width for progress bars.
 
+- `RUSTUP_DOWNLOAD_TIMEOUT` *unstable* (default: 180). Allows to override the default
+  timeout (in seconds) for downloading components.
+
 [directive syntax]: https://docs.rs/tracing-subscriber/latest/tracing_subscriber/filter/struct.EnvFilter.html#directives
 [dc]: https://docs.docker.com/storage/storagedriver/overlayfs-driver/#modifying-files-or-directories
 [override]: overrides.md

src/download/mod.rs

@@ -1,7 +1,10 @@
 //! Easy file downloading
 
 use std::fs::remove_file;
+use std::num::NonZeroU64;
 use std::path::Path;
+use std::str::FromStr;
+use std::time::Duration;
 
 use anyhow::Context;
 #[cfg(any(
@@ -194,6 +197,13 @@ async fn download_file_(
         _ => Backend::Curl,
     };
 
+    let timeout = Duration::from_secs(match process.var("RUSTUP_DOWNLOAD_TIMEOUT") {
+        Ok(s) => NonZeroU64::from_str(&s).context(
+            "invalid value in RUSTUP_DOWNLOAD_TIMEOUT -- must be a natural number greater than zero",
+        )?.get(),
+        Err(_) => 180,
+    });
+
     notify_handler(match backend {
         #[cfg(feature = "curl-backend")]
         Backend::Curl => Notification::UsingCurl,
@@ -202,7 +212,7 @@ async fn download_file_(
     });
 
     let res = backend
-        .download_to_path(url, path, resume_from_partial, Some(callback))
+        .download_to_path(url, path, resume_from_partial, Some(callback), timeout)
         .await;
 
     notify_handler(Notification::DownloadFinished);
@@ -241,9 +251,10 @@ impl Backend {
         path: &Path,
         resume_from_partial: bool,
         callback: Option<DownloadCallback<'_>>,
+        timeout: Duration,
     ) -> anyhow::Result<()> {
         let Err(err) = self
-            .download_impl(url, path, resume_from_partial, callback)
+            .download_impl(url, path, resume_from_partial, callback, timeout)
             .await
         else {
             return Ok(());
@@ -265,6 +276,7 @@ impl Backend {
         path: &Path,
         resume_from_partial: bool,
         callback: Option<DownloadCallback<'_>>,
+        timeout: Duration,
     ) -> anyhow::Result<()> {
         use std::cell::RefCell;
         use std::fs::OpenOptions;
@@ -324,7 +336,7 @@ impl Backend {
         let file = RefCell::new(file);
 
         // TODO: the sync callback will stall the async runtime if IO calls block, which is OS dependent. Rearrange.
-        self.download(url, resume_from, &|event| {
+        self.download(url, resume_from, timeout, &|event| {
             if let Event::DownloadDataReceived(data) = event {
                 file.borrow_mut()
                     .write_all(data)
@@ -356,13 +368,14 @@ impl Backend {
         self,
         url: &Url,
         resume_from: u64,
+        timeout: Duration,
         callback: DownloadCallback<'_>,
     ) -> anyhow::Result<()> {
         match self {
             #[cfg(feature = "curl-backend")]
-            Self::Curl => curl::download(url, resume_from, callback),
+            Self::Curl => curl::download(url, resume_from, callback, timeout),
             #[cfg(any(feature = "reqwest-rustls-tls", feature = "reqwest-native-tls"))]
-            Self::Reqwest(tls) => tls.download(url, resume_from, callback).await,
+            Self::Reqwest(tls) => tls.download(url, resume_from, callback, timeout).await,
         }
     }
 }
@@ -383,12 +396,13 @@ impl TlsBackend {
         url: &Url,
         resume_from: u64,
         callback: DownloadCallback<'_>,
+        timeout: Duration,
     ) -> anyhow::Result<()> {
         let client = match self {
             #[cfg(feature = "reqwest-rustls-tls")]
-            Self::Rustls => reqwest_be::rustls_client()?,
+            Self::Rustls => reqwest_be::rustls_client(timeout)?,
             #[cfg(feature = "reqwest-native-tls")]
-            Self::NativeTls => &reqwest_be::CLIENT_NATIVE_TLS,
+            Self::NativeTls => reqwest_be::native_tls_client(timeout)?,
         };
 
         reqwest_be::download(url, resume_from, callback, client).await
@@ -424,6 +438,7 @@ mod curl {
         url: &Url,
         resume_from: u64,
         callback: &dyn Fn(Event<'_>) -> Result<()>,
+        timeout: Duration,
     ) -> Result<()> {
         // Fetch either a cached libcurl handle (which will preserve open
         // connections) or create a new one if it isn't listed.
@@ -446,8 +461,8 @@ mod curl {
                 let _ = handle.resume_from(0);
             }
 
-            // Take at most 30s to connect
-            handle.connect_timeout(Duration::new(30, 0))?;
+            // Take at most 3m to connect if the `RUSTUP_DOWNLOAD_TIMEOUT` env var is not set.
+            handle.connect_timeout(timeout)?;
 
             {
                 let cberr = RefCell::new(None);
@@ -526,9 +541,7 @@ mod curl {
 #[cfg(any(feature = "reqwest-rustls-tls", feature = "reqwest-native-tls"))]
 mod reqwest_be {
     use std::io;
-    #[cfg(feature = "reqwest-native-tls")]
-    use std::sync::LazyLock;
-    #[cfg(feature = "reqwest-rustls-tls")]
+    #[cfg(any(feature = "reqwest-rustls-tls", feature = "reqwest-native-tls"))]
     use std::sync::{Arc, OnceLock};
     use std::time::Duration;
 
@@ -586,11 +599,10 @@ mod reqwest_be {
             .pool_max_idle_per_host(0)
             .gzip(false)
             .proxy(Proxy::custom(env_proxy))
-            .read_timeout(Duration::from_secs(30))
     }
 
     #[cfg(feature = "reqwest-rustls-tls")]
-    pub(super) fn rustls_client() -> Result<&'static Client, DownloadError> {
+    pub(super) fn rustls_client(timeout: Duration) -> Result<&'static Client, DownloadError> {
         if let Some(client) = CLIENT_RUSTLS_TLS.get() {
             return Ok(client);
         }
@@ -607,6 +619,7 @@ mod reqwest_be {
         tls_config.alpn_protocols = vec![b"h2".to_vec(), b"http/1.1".to_vec()];
 
         let client = client_generic()
+            .read_timeout(timeout)
             .use_preconfigured_tls(tls_config)
             .user_agent(super::REQWEST_RUSTLS_TLS_USER_AGENT)
             .build()
@@ -622,21 +635,24 @@ mod reqwest_be {
     static CLIENT_RUSTLS_TLS: OnceLock<Client> = OnceLock::new();
 
     #[cfg(feature = "reqwest-native-tls")]
-    pub(super) static CLIENT_NATIVE_TLS: LazyLock<Client> = LazyLock::new(|| {
-        let catcher = || {
-            client_generic()
-                .user_agent(super::REQWEST_DEFAULT_TLS_USER_AGENT)
-                .build()
-        };
+    pub(super) fn native_tls_client(timeout: Duration) -> Result<&'static Client, DownloadError> {
+        if let Some(client) = CLIENT_NATIVE_TLS.get() {
+            return Ok(client);
+        }
 
-        // woah, an unwrap?!
-        // It's OK. This is the same as what is happening in curl.
-        //
-        // The curl::Easy::new() internally assert!s that the initialized
-        // Easy is not null. Inside reqwest, the errors here would be from
-        // the TLS library returning a null pointer as well.
-        catcher().unwrap()
-    });
+        let client = client_generic()
+            .read_timeout(timeout)
+            .user_agent(super::REQWEST_DEFAULT_TLS_USER_AGENT)
+            .build()
+            .map_err(DownloadError::Reqwest)?;
+
+        let _ = CLIENT_NATIVE_TLS.set(client);
+
+        Ok(CLIENT_NATIVE_TLS.get().unwrap())
+    }
+
+    #[cfg(feature = "reqwest-native-tls")]
+    static CLIENT_NATIVE_TLS: OnceLock<Client> = OnceLock::new();
 
     fn env_proxy(url: &Url) -> Option<Url> {
         env_proxy::for_url(url).to_url()

src/download/tests.rs

@@ -19,6 +19,7 @@ use tempfile::TempDir;
 mod curl {
     use std::sync::Mutex;
     use std::sync::atomic::{AtomicBool, Ordering};
+    use std::time::Duration;
 
     use url::Url;
 
@@ -36,7 +37,13 @@ mod curl {
 
         let from_url = Url::from_file_path(&from_path).unwrap();
         Backend::Curl
-            .download_to_path(&from_url, &target_path, true, None)
+            .download_to_path(
+                &from_url,
+                &target_path,
+                true,
+                None,
+                Duration::from_secs(180),
+            )
             .await
             .expect("Test download failed");
 
@@ -83,6 +90,7 @@ mod curl {
 
                     Ok(())
                 }),
+                Duration::from_secs(180),
             )
             .await
             .expect("Test download failed");
@@ -185,7 +193,13 @@ mod reqwest {
 
         let from_url = Url::from_file_path(&from_path).unwrap();
         Backend::Reqwest(TlsBackend::NativeTls)
-            .download_to_path(&from_url, &target_path, true, None)
+            .download_to_path(
+                &from_url,
+                &target_path,
+                true,
+                None,
+                Duration::from_secs(180),
+            )
             .await
             .expect("Test download failed");
 
@@ -232,6 +246,7 @@ mod reqwest {
 
                     Ok(())
                 }),
+                Duration::from_secs(180),
             )
             .await
             .expect("Test download failed");