Skip to content

Commit

Permalink
Merge pull request #7774 from Turbo87/non-canonical
Browse files Browse the repository at this point in the history 
controllers/version/downloads: Move `app.version_id_cacher.insert()` call outside `spawn_blocking()`
  • Loading branch information
@Turbo87
Turbo87 authored Dec 20, 2023
2 parents 7a7b844 + 050960b commit 76927e0
Showing 1 changed file with 75 additions and 82 deletions.
157 changes: 75 additions & 82 deletions src/controllers/version/downloads.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use crate::models::VersionDownload;
use crate::schema::*;
use crate::views::EncodableVersionDownload;
use chrono::{Duration, NaiveDate, Utc};
use tokio::runtime::Handle;
use tracing::Instrument;

/// Handles the `GET /crates/:crate_id/:version/download` route.
Expand Down Expand Up @@ -39,91 +38,75 @@ pub async fn download(
} else {
app.instance_metrics.version_id_cache_misses.inc();

let app = app.clone();
let crate_name = crate_name.clone();
let version = version.clone();
spawn_blocking::<_, _, BoxedAppError>(move || {
// When no database connection is ready unconditional redirects will be performed. This could
// happen if the pool is not healthy or if an operator manually configured the application to
// always perform unconditional redirects (for example as part of the mitigations for an
// outage). See the comments below for a description of what unconditional redirects do.
let conn = if app.config.force_unconditional_redirects {
None
} else {
match app.db_read_prefer_primary() {
Ok(conn) => Some(conn),
Err(PoolError::UnhealthyPool) => None,
Err(err) => return Err(err.into()),
}
};

if let Some(mut conn) = conn {
// Returns the crate name as stored in the database, or an error if we could
// not load the version ID from the database.
let version_id = app
.instance_metrics
.downloads_select_query_execution_time
.observe_closure_duration(|| {
info_span!("db.query", message = "SELECT ... FROM versions").in_scope(
|| {
versions::table
.inner_join(crates::table)
.select(versions::id)
.filter(crates::name.eq(&crate_name))
.filter(versions::num.eq(&version))
.first::<i32>(&mut *conn)
},
)
let version_id = spawn_blocking::<_, _, BoxedAppError>({
let app = app.clone();
let crate_name = crate_name.clone();
let version = version.clone();

move || {
// When no database connection is ready unconditional redirects will be performed. This could
// happen if the pool is not healthy or if an operator manually configured the application to
// always perform unconditional redirects (for example as part of the mitigations for an
// outage). See the comments below for a description of what unconditional redirects do.
let conn = if app.config.force_unconditional_redirects {
None
} else {
match app.db_read_prefer_primary() {
Ok(conn) => Some(conn),
Err(PoolError::UnhealthyPool) => None,
Err(err) => return Err(err.into()),
}
};

if let Some(mut conn) = conn {
// Returns the crate name as stored in the database, or an error if we could
// not load the version ID from the database.
let metric = &app.instance_metrics.downloads_select_query_execution_time;
let version_id = metric.observe_closure_duration(|| {
get_version_id(&crate_name, &version, &mut conn)
})?;

// The increment does not happen instantly, but it's deferred to be executed in a batch
// along with other downloads. See crate::downloads_counter for the implementation.
app.downloads_counter.increment(version_id);

// The version_id is only cached if the provided crate name was canonical.
// Non-canonical requests fallback to the "slow" path with a DB query, but
// we typically only get a few hundred non-canonical requests in a day anyway.
let span = info_span!("cache.write", ?cache_key);

// SAFETY: This block_on should not panic. block_on will panic if the
// current thread is an executor thread of a Tokio runtime. (Will panic
// by "Cannot start a runtime from within a runtime"). Here, we are in
// a spawn_blocking call because of conduit_compat, so our current thread
// is not an executor of the runtime.
Handle::current().block_on(
app.version_id_cacher
.insert(cache_key, version_id)
.instrument(span),
);

Ok(())
} else {
// The download endpoint is the most critical route in the whole crates.io application,
// as it's relied upon by users and automations to download crates. Keeping it working
// is the most important thing for us.
//
// The endpoint relies on the database to fetch the canonical crate name (with the
// right capitalization and hyphenation), but that's only needed to serve clients who
// don't call the endpoint with the crate's canonical name.
//
// Thankfully Cargo always uses the right name when calling the endpoint, and we can
// keep it working during a full database outage by unconditionally redirecting without
// checking whether the crate exists or the right name is used. Non-Cargo clients might
// get a 404 response instead of a 500, but that's worth it.
//
// Without a working database we also can't count downloads, but that's also less
// critical than keeping Cargo downloads operational.

app.instance_metrics
.downloads_unconditional_redirects_total
.inc();

req.request_log().add("unconditional_redirect", "true");

Ok(())
// The increment does not happen instantly, but it's deferred to be executed in a batch
// along with other downloads. See crate::downloads_counter for the implementation.
app.downloads_counter.increment(version_id);

Ok(Some(version_id))
} else {
// The download endpoint is the most critical route in the whole crates.io application,
// as it's relied upon by users and automations to download crates. Keeping it working
// is the most important thing for us.
//
// The endpoint relies on the database to fetch the canonical crate name (with the
// right capitalization and hyphenation), but that's only needed to serve clients who
// don't call the endpoint with the crate's canonical name.
//
// Thankfully Cargo always uses the right name when calling the endpoint, and we can
// keep it working during a full database outage by unconditionally redirecting without
// checking whether the crate exists or the right name is used. Non-Cargo clients might
// get a 404 response instead of a 500, but that's worth it.
//
// Without a working database we also can't count downloads, but that's also less
// critical than keeping Cargo downloads operational.

app.instance_metrics
.downloads_unconditional_redirects_total
.inc();

req.request_log().add("unconditional_redirect", "true");

Ok(None)
}
}
})
.await?
.await?;

if let Some(version_id) = version_id {
let span = info_span!("cache.write", ?cache_key);
app.version_id_cacher
.insert(cache_key, version_id)
.instrument(span)
.await;
}
};

let redirect_url = app.storage.crate_location(&crate_name, &version);
Expand All @@ -134,6 +117,16 @@ pub async fn download(
}
}

#[instrument("db.query", skip(conn), fields(message = "SELECT ... FROM versions"))]
fn get_version_id(krate: &str, version: &str, conn: &mut PgConnection) -> QueryResult<i32> {
versions::table
.inner_join(crates::table)
.select(versions::id)
.filter(crates::name.eq(&krate))
.filter(versions::num.eq(&version))
.first::<i32>(conn)
}

/// Handles the `GET /crates/:crate_id/:version/downloads` route.
pub async fn downloads(
app: AppState,
Expand Down

0 comments on commit 76927e0

Please sign in to comment.