- 
                Notifications
    You must be signed in to change notification settings 
- Fork 2.7k
refactor(locking): Created locking strategy to control locking mode #16177
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.
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
|  | @@ -103,6 +103,7 @@ | |
|  | ||
| use crate::core::Workspace; | ||
| use crate::core::compiler::CompileTarget; | ||
| use crate::core::compiler::locking::{LockingMode, LockingStrategy}; | ||
| use crate::util::{CargoResult, FileLock}; | ||
| use cargo_util::paths; | ||
| use std::path::{Path, PathBuf}; | ||
|  | @@ -126,6 +127,7 @@ impl Layout { | |
| ws: &Workspace<'_>, | ||
| target: Option<CompileTarget>, | ||
| dest: &str, | ||
| locking_strategy: &LockingStrategy, | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm assuming the decision is made outside of  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the reason for this is I plan add  I could move this into  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That sounds like we should have a  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 
 We could, though it would be an  My thought was to have a struct with some semantic meaning be passed into  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 
 This sounds like a good plan! | ||
| ) -> CargoResult<Layout> { | ||
| let is_new_layout = ws.gctx().cli_unstable().build_dir_new_layout; | ||
| let mut root = ws.target_dir(); | ||
|  | @@ -152,18 +154,29 @@ impl Layout { | |
| // For now we don't do any more finer-grained locking on the artifact | ||
| // directory, so just lock the entire thing for the duration of this | ||
| // compile. | ||
| let artifact_dir_lock = | ||
| dest.open_rw_exclusive_create(".cargo-lock", ws.gctx(), "build directory")?; | ||
|  | ||
| let build_dir_lock = if root != build_root { | ||
| Some(build_dest.open_rw_exclusive_create( | ||
| let artifact_dir_lock = match locking_strategy.artifact_dir() { | ||
| LockingMode::Disabled => None, | ||
| LockingMode::Coarse => Some(dest.open_rw_exclusive_create( | ||
| ".cargo-lock", | ||
| ws.gctx(), | ||
| "build directory", | ||
| )?) | ||
| "artifact directory", | ||
| )?), | ||
| }; | ||
|  | ||
| assert_eq!(locking_strategy.is_unified_output_dir(), root == build_root); | ||
| let build_dir_lock = if !locking_strategy.is_unified_output_dir() { | ||
| match locking_strategy.build_dir() { | ||
| LockingMode::Disabled => None, | ||
| LockingMode::Coarse => Some(build_dest.open_rw_exclusive_create( | ||
| ".cargo-lock", | ||
| ws.gctx(), | ||
| "build directory", | ||
| )?), | ||
| } | ||
| } else { | ||
| None | ||
| }; | ||
|  | ||
| let root = root.into_path_unlocked(); | ||
| let build_root = build_root.into_path_unlocked(); | ||
| let dest = dest.into_path_unlocked(); | ||
|  | @@ -222,7 +235,7 @@ pub struct ArtifactDirLayout { | |
| timings: PathBuf, | ||
| /// The lockfile for a build (`.cargo-lock`). Will be unlocked when this | ||
| /// struct is `drop`ped. | ||
| _lock: FileLock, | ||
| _lock: Option<FileLock>, | ||
| } | ||
|  | ||
| impl ArtifactDirLayout { | ||
|  | ||
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| use tracing::debug; | ||
|  | ||
| use crate::{CargoResult, core::Workspace, util::flock::is_on_nfs_mount}; | ||
|  | ||
| /// The strategy to use for locking during a build. | ||
| /// | ||
| /// A strategy is made up of multiple locking modes for different directories. | ||
| #[derive(Debug)] | ||
| pub struct LockingStrategy { | ||
| /// The locking mode for the artifact-dir | ||
| artifact_dir: LockingMode, | ||
| /// The locking mode for the build-dir | ||
| /// | ||
| /// Will be `None` when artifact_dir and build_dir are the same directory. | ||
| build_dir: Option<LockingMode>, | ||
| } | ||
|  | ||
| impl LockingStrategy { | ||
| /// Determines the locking strategy the current environment can support. | ||
| 
      Comment on lines
    
      +8
     to 
      +19
    
   There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I know what change this is prep for, could we simplify it down for what is needed for this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In what way would you like to see this simplified? | ||
| pub fn determine_locking_strategy(ws: &Workspace<'_>) -> CargoResult<Self> { | ||
| let artifact_dir_locking_mode = match is_on_nfs_mount(ws.target_dir().as_path_unlocked()) { | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are we matching on  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For me find matching on  let foo = match my_bool {
   true => MyEnum::Variant1,
   false => MyEnum::Variant2,
};
// vs
let foo = if my_bool {
   MyEnum::Variant1
} else {
   MyEnum::Variant2
};Though I later added the  There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, in OCaml world matching bools is quite common. | ||
| true => { | ||
| debug!("NFS detected. Disabling file system locking for artifact-dir"); | ||
| LockingMode::Disabled | ||
| } | ||
| false => LockingMode::Coarse, | ||
| }; | ||
| let build_dir_locking_mode = if ws.target_dir() == ws.build_dir() { | ||
| None | ||
| } else { | ||
| Some(match is_on_nfs_mount(ws.build_dir().as_path_unlocked()) { | ||
| There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: To me, I prefer returning wrapped values like this to be limited to values and not expressions. It just always reads in a funny way to me and adds extra nesting on wrapping to complex expressions that could instead use them being simplified. | ||
| true => { | ||
| debug!("NFS detected. Disabling file system locking for build-dir"); | ||
| LockingMode::Disabled | ||
| } | ||
| false => LockingMode::Coarse, | ||
| }) | ||
| }; | ||
| Ok(Self { | ||
| artifact_dir: artifact_dir_locking_mode, | ||
| build_dir: build_dir_locking_mode, | ||
| }) | ||
| } | ||
|  | ||
| pub fn artifact_dir(&self) -> &LockingMode { | ||
| &self.artifact_dir | ||
| } | ||
|  | ||
| pub fn build_dir(&self) -> &LockingMode { | ||
| self.build_dir.as_ref().unwrap_or(&self.artifact_dir) | ||
| } | ||
|  | ||
| /// If the artifact_dir and build_dir are the same directory. | ||
| pub fn is_unified_output_dir(&self) -> bool { | ||
| self.build_dir.is_none() | ||
| } | ||
| } | ||
|  | ||
| /// The locking mode that will be used for output directories. | ||
| #[derive(Debug)] | ||
| pub enum LockingMode { | ||
| /// Completely disables locking (used for filesystems that do not support locking) | ||
| Disabled, | ||
| /// Coarse grain locking (Profile level) | ||
| Coarse, | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I kinda can guess why this is useful, would you mind saying it a bit clearer how this is going to be used and evolve to help the fine grained locking implementation?
(Or any comments/code I should read first in the other PR?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is to eventually add a
LockingModeenum that can be controlled by the cargo operation.The vision I have is the have 3 variants
Disabled,Coarse,Finewhich can be determined by the operation & environment.For example,
cargo cleanwould use eitherDisabledorCoarseas it does not have access to the dep graph.Another motivation is to fallback to
Coarsegrain locking if we detect thatFinegrain locking may fail. (e.g. too many file descriptors)See determine_locking_mode() in the original PR for proposed heuristics.