Skip to content

Commit

Permalink
Use a higher timeout for publishing
Browse files Browse the repository at this point in the history
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
  • Loading branch information
konstin committed Oct 4, 2024
1 parent fc49587 commit a02a574
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 22 deletions.
34 changes: 21 additions & 13 deletions crates/uv-client/src/base_client.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
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;
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;
Expand Down Expand Up @@ -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<'_> {
Expand All @@ -72,6 +73,7 @@ impl BaseClientBuilder<'_> {
markers: None,
platform: None,
auth_integration: AuthIntegration::default(),
default_timeout: Duration::from_secs(30),
}
}
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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::<u64>()
.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(
Expand Down Expand Up @@ -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 {
Expand All @@ -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.
Expand Down Expand Up @@ -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<TrustedHost>,
}
Expand Down Expand Up @@ -365,7 +373,7 @@ impl BaseClient {
}

/// The configured client timeout, in seconds.
pub fn timeout(&self) -> u64 {
pub fn timeout(&self) -> Duration {
self.timeout
}

Expand Down
16 changes: 8 additions & 8 deletions crates/uv-client/src/registry_client.rs
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -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 {
Expand All @@ -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
}

Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-distribution/src/distribution_database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
5 changes: 5 additions & 0 deletions crates/uv/src/commands/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -50,6 +51,10 @@ 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 the time a trusted publishing token is valid, so this is our
// reference.
.default_timeout(Duration::from_secs(15 * 60))
.build();
let oidc_client = BaseClientBuilder::new()
.auth_integration(AuthIntegration::NoAuthMiddleware)
Expand Down

0 comments on commit a02a574

Please sign in to comment.