Skip to content

Commit df2e2a3

Browse files
committed
Rename some Filesystem methods for clarity and consistency.
This tries to align the `Filesystem` method names so they have the same form, and also so they spell out exactly what they do. (An alternative would be to have a builder, similar to `OpenOptions`, but that seems a bit overkill here.) This also extends the docs to try to make it clearer how this works.
1 parent fd61851 commit df2e2a3

File tree

8 files changed

+107
-42
lines changed

8 files changed

+107
-42
lines changed

src/cargo/core/compiler/future_incompat.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ impl OnDiskReports {
167167
let on_disk = serde_json::to_vec(&self).unwrap();
168168
if let Err(e) = ws
169169
.target_dir()
170-
.open_rw(
170+
.open_rw_exclusive_create(
171171
FUTURE_INCOMPAT_FILE,
172172
ws.config(),
173173
"Future incompatibility report",
@@ -191,7 +191,7 @@ impl OnDiskReports {
191191

192192
/// Loads the on-disk reports.
193193
pub fn load(ws: &Workspace<'_>) -> CargoResult<OnDiskReports> {
194-
let report_file = match ws.target_dir().open_ro(
194+
let report_file = match ws.target_dir().open_ro_shared(
195195
FUTURE_INCOMPAT_FILE,
196196
ws.config(),
197197
"Future incompatible report",

src/cargo/core/compiler/layout.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ impl Layout {
166166
// For now we don't do any more finer-grained locking on the artifact
167167
// directory, so just lock the entire thing for the duration of this
168168
// compile.
169-
let lock = dest.open_rw(".cargo-lock", ws.config(), "build directory")?;
169+
let lock = dest.open_rw_exclusive_create(".cargo-lock", ws.config(), "build directory")?;
170170
let root = root.into_path_unlocked();
171171
let dest = dest.into_path_unlocked();
172172
let deps = dest.join("deps");

src/cargo/ops/cargo_package.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -133,7 +133,7 @@ pub fn package_one(
133133
let dir = ws.target_dir().join("package");
134134
let mut dst = {
135135
let tmp = format!(".{}", filename);
136-
dir.open_rw(&tmp, config, "package scratch space")?
136+
dir.open_rw_exclusive_create(&tmp, config, "package scratch space")?
137137
};
138138

139139
// Package up and test a temporary tarball and only move it to the final

src/cargo/ops/common_for_install_and_uninstall.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,10 @@ pub struct CrateListingV1 {
9898
impl InstallTracker {
9999
/// Create an InstallTracker from information on disk.
100100
pub fn load(config: &Config, root: &Filesystem) -> CargoResult<InstallTracker> {
101-
let v1_lock = root.open_rw(Path::new(".crates.toml"), config, "crate metadata")?;
102-
let v2_lock = root.open_rw(Path::new(".crates2.json"), config, "crate metadata")?;
101+
let v1_lock =
102+
root.open_rw_exclusive_create(Path::new(".crates.toml"), config, "crate metadata")?;
103+
let v2_lock =
104+
root.open_rw_exclusive_create(Path::new(".crates2.json"), config, "crate metadata")?;
103105

104106
let v1 = (|| -> CargoResult<_> {
105107
let mut contents = String::new();

src/cargo/ops/lockfile.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ pub fn load_pkg_lockfile(ws: &Workspace<'_>) -> CargoResult<Option<Resolve>> {
1212
return Ok(None);
1313
}
1414

15-
let mut f = lock_root.open_ro("Cargo.lock", ws.config(), "Cargo.lock file")?;
15+
let mut f = lock_root.open_ro_shared("Cargo.lock", ws.config(), "Cargo.lock file")?;
1616

1717
let mut s = String::new();
1818
f.read_to_string(&mut s)
@@ -79,7 +79,7 @@ pub fn write_pkg_lockfile(ws: &Workspace<'_>, resolve: &mut Resolve) -> CargoRes
7979

8080
// Ok, if that didn't work just write it out
8181
lock_root
82-
.open_rw("Cargo.lock", ws.config(), "Cargo.lock file")
82+
.open_rw_exclusive_create("Cargo.lock", ws.config(), "Cargo.lock file")
8383
.and_then(|mut f| {
8484
f.file().set_len(0)?;
8585
f.write_all(out.as_bytes())?;
@@ -100,7 +100,7 @@ fn resolve_to_string_orig(
100100
) -> (Option<String>, String, Filesystem) {
101101
// Load the original lock file if it exists.
102102
let lock_root = lock_root(ws);
103-
let orig = lock_root.open_ro("Cargo.lock", ws.config(), "Cargo.lock file");
103+
let orig = lock_root.open_ro_shared("Cargo.lock", ws.config(), "Cargo.lock file");
104104
let orig = orig.and_then(|mut f| {
105105
let mut s = String::new();
106106
f.read_to_string(&mut s)?;

src/cargo/util/cache_lock.rs

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -188,19 +188,20 @@ impl RecursiveLock {
188188
fn lock_shared_blocking(&mut self, config: &Config, description: &'static str) {
189189
if self.count == 0 {
190190
self.is_exclusive = false;
191-
self.lock = match config
192-
.home()
193-
.open_shared_create(self.filename, config, description)
194-
{
195-
Ok(lock) => Some(lock),
196-
Err(e) => {
197-
// There is no error here because locking is mostly a
198-
// best-effort attempt. If cargo home is read-only, we don't
199-
// want to fail just because we couldn't create the lock file.
200-
tracing::warn!("failed to acquire cache lock {}: {e:?}", self.filename);
201-
None
202-
}
203-
};
191+
self.lock =
192+
match config
193+
.home()
194+
.open_ro_shared_create(self.filename, config, description)
195+
{
196+
Ok(lock) => Some(lock),
197+
Err(e) => {
198+
// There is no error here because locking is mostly a
199+
// best-effort attempt. If cargo home is read-only, we don't
200+
// want to fail just because we couldn't create the lock file.
201+
tracing::warn!("failed to acquire cache lock {}: {e:?}", self.filename);
202+
None
203+
}
204+
};
204205
}
205206
self.increment();
206207
}
@@ -209,7 +210,7 @@ impl RecursiveLock {
209210
fn lock_shared_nonblocking(&mut self, config: &Config) -> LockingResult {
210211
if self.count == 0 {
211212
self.is_exclusive = false;
212-
self.lock = match config.home().try_open_shared_create(self.filename) {
213+
self.lock = match config.home().try_open_ro_shared_create(self.filename) {
213214
Ok(Some(lock)) => Some(lock),
214215
Ok(None) => {
215216
return WouldBlock;
@@ -255,7 +256,10 @@ impl RecursiveLock {
255256
) -> CargoResult<()> {
256257
if self.count == 0 {
257258
self.is_exclusive = true;
258-
match config.home().open_rw(self.filename, config, description) {
259+
match config
260+
.home()
261+
.open_rw_exclusive_create(self.filename, config, description)
262+
{
259263
Ok(lock) => self.lock = Some(lock),
260264
Err(e) => {
261265
if maybe_readonly(&e) {
@@ -288,7 +292,7 @@ impl RecursiveLock {
288292
fn lock_exclusive_nonblocking(&mut self, config: &Config) -> CargoResult<LockingResult> {
289293
if self.count == 0 {
290294
self.is_exclusive = true;
291-
match config.home().try_open_rw(self.filename) {
295+
match config.home().try_open_rw_exclusive_create(self.filename) {
292296
Ok(Some(lock)) => self.lock = Some(lock),
293297
Ok(None) => return Ok(WouldBlock),
294298
Err(e) => {

src/cargo/util/config/mod.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2235,7 +2235,7 @@ pub fn save_credentials(
22352235
let mut file = {
22362236
cfg.home_path.create_dir()?;
22372237
cfg.home_path
2238-
.open_rw(filename, cfg, "credentials' config file")?
2238+
.open_rw_exclusive_create(filename, cfg, "credentials' config file")?
22392239
};
22402240

22412241
let mut contents = String::new();

src/cargo/util/flock.rs

Lines changed: 75 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,12 @@
1+
//! File-locking support.
2+
//!
3+
//! This module defines the [`Filesystem`] type which is an abstraction over a
4+
//! filesystem, ensuring that access to the filesystem is only done through
5+
//! coordinated locks.
6+
//!
7+
//! The [`FileLock`] type represents a locked file, and provides access to the
8+
//! file.
9+
110
use std::fs::{File, OpenOptions};
211
use std::io;
312
use std::io::{Read, Seek, SeekFrom, Write};
@@ -10,6 +19,18 @@ use anyhow::Context as _;
1019
use cargo_util::paths;
1120
use sys::*;
1221

22+
/// A locked file.
23+
///
24+
/// This provides access to file while holding a lock on the file. This type
25+
/// implements the [`Read`], [`Write`], and [`Seek`] traits to provide access
26+
/// to the underlying file.
27+
///
28+
/// Locks are either shared (multiple processes can access the file) or
29+
/// exclusive (only one process can access the file).
30+
///
31+
/// This type is created via methods on the [`Filesystem`] type.
32+
///
33+
/// When this value is dropped, the lock will be released.
1334
#[derive(Debug)]
1435
pub struct FileLock {
1536
f: Option<File>,
@@ -95,6 +116,32 @@ impl Drop for FileLock {
95116
/// The `Path` of a filesystem cannot be learned unless it's done in a locked
96117
/// fashion, and otherwise functions on this structure are prepared to handle
97118
/// concurrent invocations across multiple instances of Cargo.
119+
///
120+
/// The methods on `Filesystem` that open files return a [`FileLock`] which
121+
/// holds the lock, and that type provides methods for accessing the
122+
/// underlying file.
123+
///
124+
/// If the blocking methods (like [`Filesystem::open_ro_shared`]) detect that
125+
/// they will block, then they will display a message to the user letting them
126+
/// know it is blocked. There are non-blocking variants starting with the
127+
/// `try_` prefix like [`Filesystem::try_open_ro_shared_create`].
128+
///
129+
/// The behavior of locks acquired by the `Filesystem` depend on the operating
130+
/// system. On unix-like system, they are advisory using [`flock`], and thus
131+
/// not enforced against processes which do not try to acquire the lock. On
132+
/// Windows, they are mandatory using [`LockFileEx`], enforced against all
133+
/// processes.
134+
///
135+
/// This **does not** guarantee that a lock is acquired. In some cases, for
136+
/// example on filesystems that don't support locking, it will return a
137+
/// [`FileLock`] even though the filesystem lock was not acquired. This is
138+
/// intended to provide a graceful fallback instead of refusing to work.
139+
/// Usually there aren't multiple processes accessing the same resource. In
140+
/// that case, it is the user's responsibility to not run concurrent
141+
/// processes.
142+
///
143+
/// [`flock`]: https://linux.die.net/man/2/flock
144+
/// [`LockFileEx`]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-lockfileex
98145
#[derive(Clone, Debug)]
99146
pub struct Filesystem {
100147
root: PathBuf,
@@ -147,17 +194,22 @@ impl Filesystem {
147194
self.root.display()
148195
}
149196

150-
/// Opens exclusive access to a file, returning the locked version of a
151-
/// file.
197+
/// Opens read-write exclusive access to a file, returning the locked
198+
/// version of a file.
152199
///
153200
/// This function will create a file at `path` if it doesn't already exist
154201
/// (including intermediate directories), and then it will acquire an
155202
/// exclusive lock on `path`. If the process must block waiting for the
156-
/// lock, the `msg` is printed to `config`.
203+
/// lock, the `msg` is printed to [`Config`].
157204
///
158205
/// The returned file can be accessed to look at the path and also has
159206
/// read/write access to the underlying file.
160-
pub fn open_rw<P>(&self, path: P, config: &Config, msg: &str) -> CargoResult<FileLock>
207+
pub fn open_rw_exclusive_create<P>(
208+
&self,
209+
path: P,
210+
config: &Config,
211+
msg: &str,
212+
) -> CargoResult<FileLock>
161213
where
162214
P: AsRef<Path>,
163215
{
@@ -170,11 +222,14 @@ impl Filesystem {
170222
Ok(FileLock { f: Some(f), path })
171223
}
172224

173-
/// A non-blocking version of [`Filesystem::open_rw`].
225+
/// A non-blocking version of [`Filesystem::open_rw_exclusive_create`].
174226
///
175227
/// Returns `None` if the operation would block due to another process
176228
/// holding the lock.
177-
pub fn try_open_rw<P: AsRef<Path>>(&self, path: P) -> CargoResult<Option<FileLock>> {
229+
pub fn try_open_rw_exclusive_create<P: AsRef<Path>>(
230+
&self,
231+
path: P,
232+
) -> CargoResult<Option<FileLock>> {
178233
let mut opts = OpenOptions::new();
179234
opts.read(true).write(true).create(true);
180235
let (path, f) = self.open(path.as_ref(), &opts, true)?;
@@ -185,16 +240,16 @@ impl Filesystem {
185240
}
186241
}
187242

188-
/// Opens shared access to a file, returning the locked version of a file.
243+
/// Opens read-only shared access to a file, returning the locked version of a file.
189244
///
190245
/// This function will fail if `path` doesn't already exist, but if it does
191246
/// then it will acquire a shared lock on `path`. If the process must block
192-
/// waiting for the lock, the `msg` is printed to `config`.
247+
/// waiting for the lock, the `msg` is printed to [`Config`].
193248
///
194249
/// The returned file can be accessed to look at the path and also has read
195250
/// access to the underlying file. Any writes to the file will return an
196251
/// error.
197-
pub fn open_ro<P>(&self, path: P, config: &Config, msg: &str) -> CargoResult<FileLock>
252+
pub fn open_ro_shared<P>(&self, path: P, config: &Config, msg: &str) -> CargoResult<FileLock>
198253
where
199254
P: AsRef<Path>,
200255
{
@@ -205,11 +260,12 @@ impl Filesystem {
205260
Ok(FileLock { f: Some(f), path })
206261
}
207262

208-
/// Opens shared access to a file, returning the locked version of a file.
263+
/// Opens read-only shared access to a file, returning the locked version of a file.
209264
///
210-
/// Compared to [`Filesystem::open_ro`], this will create the file (and
211-
/// any directories in the parent) if the file does not already exist.
212-
pub fn open_shared_create<P: AsRef<Path>>(
265+
/// Compared to [`Filesystem::open_ro_shared`], this will create the file
266+
/// (and any directories in the parent) if the file does not already
267+
/// exist.
268+
pub fn open_ro_shared_create<P: AsRef<Path>>(
213269
&self,
214270
path: P,
215271
config: &Config,
@@ -224,11 +280,14 @@ impl Filesystem {
224280
Ok(FileLock { f: Some(f), path })
225281
}
226282

227-
/// A non-blocking version of [`Filesystem::open_shared_create`].
283+
/// A non-blocking version of [`Filesystem::open_ro_shared_create`].
228284
///
229285
/// Returns `None` if the operation would block due to another process
230286
/// holding the lock.
231-
pub fn try_open_shared_create<P: AsRef<Path>>(&self, path: P) -> CargoResult<Option<FileLock>> {
287+
pub fn try_open_ro_shared_create<P: AsRef<Path>>(
288+
&self,
289+
path: P,
290+
) -> CargoResult<Option<FileLock>> {
232291
let mut opts = OpenOptions::new();
233292
opts.read(true).write(true).create(true);
234293
let (path, f) = self.open(path.as_ref(), &opts, true)?;
@@ -316,7 +375,7 @@ fn try_acquire(path: &Path, lock_try: &dyn Fn() -> io::Result<()>) -> CargoResul
316375
/// This function will acquire the lock on a `path`, printing out a nice message
317376
/// to the console if we have to wait for it. It will first attempt to use `try`
318377
/// to acquire a lock on the crate, and in the case of contention it will emit a
319-
/// status message based on `msg` to `config`'s shell, and then use `block` to
378+
/// status message based on `msg` to [`Config`]'s shell, and then use `block` to
320379
/// block waiting to acquire a lock.
321380
///
322381
/// Returns an error if the lock could not be acquired or if any error other

0 commit comments

Comments
 (0)