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

Use a flock to avoid concurrent initialization of project environments #11259

Merged
merged 1 commit into from
Feb 5, 2025
+17 −1
Merged

Use a flock to avoid concurrent initialization of project environments #11259

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
18 changes: 17 additions & 1 deletion crates/uv/src/commands/project/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use owo_colors::OwoColorize;
use tracing::debug;

use uv_cache::Cache;
use uv_cache_key::cache_digest;
use uv_client::{BaseClientBuilder, Connectivity, FlatIndexClient, RegistryClientBuilder};
use uv_configuration::{
Concurrency, Constraints, DevGroupsManifest, DevGroupsSpecification, ExtrasSpecification,
Expand All @@ -18,7 +19,7 @@ use uv_distribution::DistributionDatabase;
use uv_distribution_types::{
Index, Resolution, UnresolvedRequirement, UnresolvedRequirementSpecification,
};
use uv_fs::{Simplified, CWD};
use uv_fs::{LockedFile, Simplified, CWD};
use uv_git::ResolvedRepositoryReference;
use uv_installer::{SatisfiesResult, SitePackages};
use uv_normalize::{GroupName, PackageName, DEV_DEPENDENCIES};
Expand Down Expand Up @@ -743,6 +744,18 @@ impl ProjectInterpreter {
ProjectInterpreter::Environment(venv) => venv.into_interpreter(),
}
}

/// Grab a file lock for the environment to prevent concurrent writes across processes.
pub(crate) async fn lock(workspace: &Workspace) -> Result<LockedFile, std::io::Error> {
LockedFile::acquire(
std::env::temp_dir().join(format!(
Copy link
Member

@zanieb zanieb Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't this naughty? Aren't you supposed to grab some uv-specific tmp dir location?

(I thought we removed these intentionally in the past, the only remaining reference is in the trampolines)

Copy link
Member

Choose a reason for hiding this comment

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

Oh and

uv/crates/uv-python/src/environment.rs

Line 311 in d281f49

env::temp_dir().join(format!("uv-{}.lock", cache_digest(&self.0.root))),

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we do this elsewhere. I guess we could create a directory within env::temp_dir called uv?

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly I kind of think we should do this everywhere rather than creating .lock files in virtualenvs, etc.

Copy link
Member

Choose a reason for hiding this comment

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

I thought there was a very intentional reason we used co-located temporary files or the cache instead of /tmp

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if it's relevant here but OS tempdirs are popular sources of Different Filesystem, and moving a file across filesystems does not benefit from the same atomicity guarantees (it's like cp instead of mv).

Copy link
Contributor

@Gankra Gankra Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

(This is broadly possible with basically any random Other Directory, hence "do atomic moves/renames only in the ~same directory" is often advisable)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think this is where we landed in Discord — co-location is critical for any atomic rename operations.

"uv-{}.lock",
cache_digest(workspace.install_path())
)),
workspace.install_path().user_display(),
)
.await
}
}

/// The source of a `Requires-Python` specifier.
Expand Down Expand Up @@ -929,6 +942,9 @@ pub(crate) async fn get_or_init_environment(
cache: &Cache,
printer: Printer,
) -> Result<PythonEnvironment, ProjectError> {
// Lock the project environment to avoid synchronization issues.
let _lock = ProjectInterpreter::lock(workspace).await?;

match ProjectInterpreter::discover(
workspace,
workspace.install_path().as_ref(),
Expand Down
Loading