From c8dabcb6cd0759de9326bf1c32d64b1a87eef47e Mon Sep 17 00:00:00 2001 From: konstin Date: Fri, 4 Oct 2024 13:15:12 +0200 Subject: [PATCH] Use a higher timeout for publishing A 30s timeout is too low when uploading a large package over a home internet connection, where uploads can be 10x slower than downloads. We increase the default limit to 15min, while keeping the environment variables to override this intact. In the process, I've lifted the conversion of the timeout seconds to `Duration` up. `Duration::from_mins` is nightly only so we don't quite get the benefit yet. Fixes #7809 --- crates/uv-client/src/base_client.rs | 34 ++++++++++++------- crates/uv-client/src/registry_client.rs | 16 ++++----- .../src/distribution_database.rs | 2 +- crates/uv/src/commands/publish.rs | 4 +++ 4 files changed, 34 insertions(+), 22 deletions(-) diff --git a/crates/uv-client/src/base_client.rs b/crates/uv-client/src/base_client.rs index de883cbc748e..ad6f55b75699 100644 --- a/crates/uv-client/src/base_client.rs +++ b/crates/uv-client/src/base_client.rs @@ -1,8 +1,3 @@ -use std::error::Error; -use std::fmt::Debug; -use std::path::Path; -use std::{env, iter}; - use itertools::Itertools; use reqwest::{Client, ClientBuilder, Response}; use reqwest_middleware::ClientWithMiddleware; @@ -10,6 +5,11 @@ use reqwest_retry::policies::ExponentialBackoff; use reqwest_retry::{ DefaultRetryableStrategy, RetryTransientMiddleware, Retryable, RetryableStrategy, }; +use std::error::Error; +use std::fmt::Debug; +use std::path::Path; +use std::time::Duration; +use std::{env, iter}; use tracing::debug; use url::Url; use uv_auth::AuthMiddleware; @@ -52,6 +52,7 @@ pub struct BaseClientBuilder<'a> { markers: Option<&'a MarkerEnvironment>, platform: Option<&'a Platform>, auth_integration: AuthIntegration, + default_timeout: Duration, } impl Default for BaseClientBuilder<'_> { @@ -72,6 +73,7 @@ impl BaseClientBuilder<'_> { markers: None, platform: None, auth_integration: AuthIntegration::default(), + default_timeout: Duration::from_secs(30), } } } @@ -131,6 +133,12 @@ impl<'a> BaseClientBuilder<'a> { self } + #[must_use] + pub fn default_timeout(mut self, default_timeout: Duration) -> Self { + self.default_timeout = default_timeout; + self + } + pub fn is_offline(&self) -> bool { matches!(self.connectivity, Connectivity::Offline) } @@ -161,20 +169,20 @@ impl<'a> BaseClientBuilder<'a> { // Timeout options, matching https://doc.rust-lang.org/nightly/cargo/reference/config.html#httptimeout // `UV_REQUEST_TIMEOUT` is provided for backwards compatibility with v0.1.6 - let default_timeout = 30; let timeout = env::var("UV_HTTP_TIMEOUT") .or_else(|_| env::var("UV_REQUEST_TIMEOUT")) .or_else(|_| env::var("HTTP_TIMEOUT")) .and_then(|value| { value.parse::() + .map(Duration::from_secs) .or_else(|_| { // On parse error, warn and use the default timeout warn_user_once!("Ignoring invalid value from environment for `UV_HTTP_TIMEOUT`. Expected an integer number of seconds, got \"{value}\"."); - Ok(default_timeout) + Ok(self.default_timeout) }) }) - .unwrap_or(default_timeout); - debug!("Using request timeout of {timeout}s"); + .unwrap_or(self.default_timeout); + debug!("Using request timeout of {}s", timeout.as_secs()); // Create a secure client that validates certificates. let raw_client = self.create_client( @@ -227,7 +235,7 @@ impl<'a> BaseClientBuilder<'a> { fn create_client( &self, user_agent: &str, - timeout: u64, + timeout: Duration, ssl_cert_file_exists: bool, security: Security, ) -> Client { @@ -236,7 +244,7 @@ impl<'a> BaseClientBuilder<'a> { .http1_title_case_headers() .user_agent(user_agent) .pool_max_idle_per_host(20) - .read_timeout(std::time::Duration::from_secs(timeout)) + .read_timeout(timeout) .tls_built_in_root_certs(false); // If necessary, accept invalid certificates. @@ -327,7 +335,7 @@ pub struct BaseClient { /// The connectivity mode to use. connectivity: Connectivity, /// Configured client timeout, in seconds. - timeout: u64, + timeout: Duration, /// Hosts that are trusted to use the insecure client. allow_insecure_host: Vec, } @@ -365,7 +373,7 @@ impl BaseClient { } /// The configured client timeout, in seconds. - pub fn timeout(&self) -> u64 { + pub fn timeout(&self) -> Duration { self.timeout } diff --git a/crates/uv-client/src/registry_client.rs b/crates/uv-client/src/registry_client.rs index 8cc846f3e388..f98ca7f8c23a 100644 --- a/crates/uv-client/src/registry_client.rs +++ b/crates/uv-client/src/registry_client.rs @@ -1,13 +1,13 @@ -use std::collections::BTreeMap; -use std::fmt::Debug; -use std::path::PathBuf; -use std::str::FromStr; - use async_http_range_reader::AsyncHttpRangeReader; use futures::{FutureExt, TryStreamExt}; use http::HeaderMap; use reqwest::{Client, Response, StatusCode}; use reqwest_middleware::ClientWithMiddleware; +use std::collections::BTreeMap; +use std::fmt::Debug; +use std::path::PathBuf; +use std::str::FromStr; +use std::time::Duration; use tracing::{info_span, instrument, trace, warn, Instrument}; use url::Url; @@ -171,7 +171,7 @@ pub struct RegistryClient { /// The connectivity mode to use. connectivity: Connectivity, /// Configured client timeout, in seconds. - timeout: u64, + timeout: Duration, } impl RegistryClient { @@ -191,7 +191,7 @@ impl RegistryClient { } /// Return the timeout this client is configured with, in seconds. - pub fn timeout(&self) -> u64 { + pub fn timeout(&self) -> Duration { self.timeout } @@ -713,7 +713,7 @@ impl RegistryClient { std::io::Error::new( std::io::ErrorKind::TimedOut, format!( - "Failed to download distribution due to network timeout. Try increasing UV_HTTP_TIMEOUT (current value: {}s).", self.timeout() + "Failed to download distribution due to network timeout. Try increasing UV_HTTP_TIMEOUT (current value: {}s).", self.timeout().as_secs() ), ) } else { diff --git a/crates/uv-distribution/src/distribution_database.rs b/crates/uv-distribution/src/distribution_database.rs index 7350de08fd21..05db4ce8da11 100644 --- a/crates/uv-distribution/src/distribution_database.rs +++ b/crates/uv-distribution/src/distribution_database.rs @@ -87,7 +87,7 @@ impl<'a, Context: BuildContext> DistributionDatabase<'a, Context> { io::Error::new( io::ErrorKind::TimedOut, format!( - "Failed to download distribution due to network timeout. Try increasing UV_HTTP_TIMEOUT (current value: {}s).", self.client.unmanaged.timeout() + "Failed to download distribution due to network timeout. Try increasing UV_HTTP_TIMEOUT (current value: {}s).", self.client.unmanaged.timeout().as_secs() ), ) } else { diff --git a/crates/uv/src/commands/publish.rs b/crates/uv/src/commands/publish.rs index 50f7f3c40e1e..38e2e19893ed 100644 --- a/crates/uv/src/commands/publish.rs +++ b/crates/uv/src/commands/publish.rs @@ -5,6 +5,7 @@ use anyhow::{bail, Result}; use owo_colors::OwoColorize; use std::fmt::Write; use std::sync::Arc; +use std::time::Duration; use tracing::info; use url::Url; use uv_client::{AuthIntegration, BaseClientBuilder, Connectivity, DEFAULT_RETRIES}; @@ -50,6 +51,9 @@ pub(crate) async fn publish( .allow_insecure_host(allow_insecure_host) // Don't try cloning the request to make an unauthenticated request first. .auth_integration(AuthIntegration::OnlyAuthenticated) + // Set a very high timeout for uploads, connections are often 10x slower on upload than + // download. 15 min is taken from the time a trusted publishing token is valid. + .default_timeout(Duration::from_secs(15 * 60)) .build(); let oidc_client = BaseClientBuilder::new() .auth_integration(AuthIntegration::NoAuthMiddleware)