Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update dependency moka to v0.12.1 #7075

Merged
merged 1 commit into from
Oct 16, 2023
Merged

Update dependency moka to v0.12.1 #7075

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 8 additions & 121 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ ipnetwork = "=0.20.0"
tikv-jemallocator = { version = "=0.5.4", features = ['unprefixed_malloc_on_supported_platforms', 'profiling'] }
lettre = { version = "=0.11.0", default-features = false, features = ["file-transport", "smtp-transport", "native-tls", "hostname", "builder"] }
minijinja = "=1.0.8"
moka = { version = "=0.11.3", features = ["future"] }
moka = { version = "=0.12.1", features = ["future"] }
oauth2 = { version = "=4.4.2", default-features = false, features = ["reqwest"] }
object_store = { version = "=0.7.1", features = ["aws"] }
once_cell = "=1.18.0"
Expand Down
18 changes: 13 additions & 5 deletions src/controllers/version/downloads.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::models::{Crate, VersionDownload};
use crate::schema::*;
use crate::views::EncodableVersionDownload;
use chrono::{Duration, NaiveDate, Utc};
use tracing::Instrument;

/// Handles the `GET /crates/:crate_id/:version/download` route.
/// This returns a URL to the location where the crate is stored.
Expand All @@ -22,8 +23,11 @@ pub async fn download(

let cache_key = (crate_name.to_string(), version.to_string());

let cache_result =
info_span!("cache.read", ?cache_key).in_scope(|| app.version_id_cacher.get(&cache_key));
let cache_result = app
.version_id_cacher
.get(&cache_key)
.instrument(info_span!("cache.read", ?cache_key))
.await;

let (crate_name, version) = if let Some(version_id) = cache_result {
app.instance_metrics.version_id_cache_hits.inc();
Expand All @@ -37,6 +41,7 @@ pub async fn download(
app.instance_metrics.version_id_cache_misses.inc();

let app = app.clone();
let rt = tokio::runtime::Handle::current();
conduit_compat(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
Expand Down Expand Up @@ -89,9 +94,12 @@ pub async fn download(
// 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.
info_span!("cache.write", ?cache_key).in_scope(|| {
app.version_id_cacher
.blocking()
.insert(cache_key, version_id)
// 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.
rt.block_on(app.version_id_cacher.insert(cache_key, version_id))
Copy link
Member

@Turbo87 Turbo87 Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tatsuya6502 is there a reason why you extracted the rt variable outside of the conduit_compat() closure instead of using Handle::current().block_on(...) directly here? we've been using Handle::current().block_on(...) in a few other places and I'm trying to figure out if that is a mistake or not 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi. I did not realize that you already had Handle::current().block_on(...) in other places. I never tried doing so, and I am not sure if it is okay or not.

I had an impression that Handle::current() should be called in async context. But I just checked Tokio's document and it seems I was wrong. It says the followings:

https://docs.rs/tokio/1.33.0/tokio/runtime/struct.Handle.html#method.current

pub fn current() -> Self

Returns a Handle view over the currently running Runtime.

Panics

This will panic if called outside the context of a Tokio runtime. That means that you must call this on one of the threads being run by the runtime, or from a thread with an active EnterGuard.

It says "outside the context of Tokio runtime", not "async context". conduit_compat runs the given closure in tokio::task::spawn_blocking, which uses a pool of threads being run by Tokio runtime. So, if understand correctly, calling Handle::current() should be okay there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay, thanks for the clarification. I guess we'll try it out and see what happens. The test suite seems to work fine that way at least.

});

Ok((crate_name, version))
Expand Down