Skip to content

Commit c86bc37

Browse files
authored
Make GlobalContext Sync (#15967)
### What does this PR try to resolve? By making `GlobalContext` `Sync`, it will become much easier to parallelize parts of cargo. A concrete example is #15676. It was my understanding from #15934 (comment) that this would be a welcome change by the cargo team. #### Overview In `GlobalContext` and structs used in its fields - `RefCell`-s were replaced by either an `std::sync::Mutex` or an `std::sync::RwLock`, depending on API needs - `LazyCell`-s were replaced by a new `OnceLock` implementation backed by `std::sync::OnceLock`, emulating unstable features needed by cargo - Removed `LazyCell`/`OnceLock` from fields where the initialization function is just a `Mutex<HashMap>::default()` - added `util::context::tests::sync_context` test that does not compile if `GlobalContext` is not `Sync` ### How to test and review this PR? This PR should add no user-facing changes. Tests must pass and benchmarks must report no changes.
2 parents b624a44 + 9929c87 commit c86bc37

File tree

10 files changed

+211
-104
lines changed

10 files changed

+211
-104
lines changed

src/cargo/core/registry.rs

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -800,11 +800,9 @@ impl<'gctx> Registry for PackageRegistry<'gctx> {
800800

801801
#[tracing::instrument(skip_all)]
802802
fn block_until_ready(&mut self) -> CargoResult<()> {
803-
if cfg!(debug_assertions) {
804-
// Force borrow to catch invalid borrows, regardless of which source is used and how it
805-
// happens to behave this time
806-
self.gctx.shell().verbosity();
807-
}
803+
// Ensure `shell` is not already in use,
804+
// regardless of which source is used and how it happens to behave this time
805+
self.gctx.debug_assert_shell_not_borrowed();
808806
for (source_id, source) in self.sources.sources_mut() {
809807
source
810808
.block_until_ready()

src/cargo/core/shell.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@ impl Shell {
6565
}
6666

6767
/// Creates a shell from a plain writable object, with no color, and max verbosity.
68-
pub fn from_write(out: Box<dyn Write>) -> Shell {
68+
pub fn from_write(out: Box<dyn Write + Send + Sync>) -> Shell {
6969
Shell {
7070
output: ShellOut::Write(AutoStream::never(out)), // strip all formatting on write
7171
verbosity: Verbosity::Verbose,
@@ -432,7 +432,7 @@ impl Default for Shell {
432432
/// A `Write`able object, either with or without color support
433433
enum ShellOut {
434434
/// A plain write object without color support
435-
Write(AutoStream<Box<dyn Write>>),
435+
Write(AutoStream<Box<dyn Write + Send + Sync>>),
436436
/// Color-enabled stdio, with information on whether color should be used
437437
Stream {
438438
stdout: AutoStream<std::io::Stdout>,

src/cargo/core/workspace.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2037,7 +2037,7 @@ fn find_workspace_root_with_loader(
20372037
) -> CargoResult<Option<PathBuf>> {
20382038
// Check if there are any workspace roots that have already been found that would work
20392039
{
2040-
let roots = gctx.ws_roots.borrow();
2040+
let roots = gctx.ws_roots();
20412041
// Iterate through the manifests parent directories until we find a workspace
20422042
// root. Note we skip the first item since that is just the path itself
20432043
for current in manifest_path.ancestors().skip(1) {

src/cargo/sources/git/utils.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1528,7 +1528,7 @@ fn github_fast_path(
15281528
"https://api.github.com/repos/{}/{}/commits/{}",
15291529
username, repository, github_branch_name,
15301530
);
1531-
let mut handle = gctx.http()?.borrow_mut();
1531+
let mut handle = gctx.http()?.lock().unwrap();
15321532
debug!("attempting GitHub fast path for {}", url);
15331533
handle.get(true)?;
15341534
handle.url(&url)?;

src/cargo/util/cache_lock.rs

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,8 @@ use super::FileLock;
9191
use crate::CargoResult;
9292
use crate::GlobalContext;
9393
use anyhow::Context as _;
94-
use std::cell::RefCell;
9594
use std::io;
95+
use std::sync::Mutex;
9696

9797
/// The style of lock to acquire.
9898
#[derive(Copy, Clone, Debug, PartialEq)]
@@ -435,7 +435,11 @@ pub struct CacheLock<'lock> {
435435
impl Drop for CacheLock<'_> {
436436
fn drop(&mut self) {
437437
use CacheLockMode::*;
438-
let mut state = self.locker.state.borrow_mut();
438+
let mut state = match self.locker.state.lock() {
439+
Ok(result) => result,
440+
// we should release the cache even if a thread panicked while holding a lock
441+
Err(poison) => poison.into_inner(),
442+
};
439443
match self.mode {
440444
Shared => {
441445
state.mutate_lock.decrement();
@@ -472,24 +476,25 @@ pub struct CacheLocker {
472476
///
473477
/// [`CacheLocker`] uses interior mutability because it is stuffed inside
474478
/// [`GlobalContext`], which does not allow mutation.
475-
state: RefCell<CacheState>,
479+
state: Mutex<CacheState>,
476480
}
477481

478482
impl CacheLocker {
479483
/// Creates a new `CacheLocker`.
480484
pub fn new() -> CacheLocker {
481485
CacheLocker {
482-
state: RefCell::new(CacheState {
486+
state: CacheState {
483487
cache_lock: RecursiveLock::new(CACHE_LOCK_NAME),
484488
mutate_lock: RecursiveLock::new(MUTATE_NAME),
485-
}),
489+
}
490+
.into(),
486491
}
487492
}
488493

489494
/// Acquires a lock with the given mode, possibly blocking if another
490495
/// cargo is holding the lock.
491496
pub fn lock(&self, gctx: &GlobalContext, mode: CacheLockMode) -> CargoResult<CacheLock<'_>> {
492-
let mut state = self.state.borrow_mut();
497+
let mut state = self.state.lock().unwrap();
493498
let _ = state.lock(gctx, mode, Blocking)?;
494499
Ok(CacheLock { mode, locker: self })
495500
}
@@ -501,7 +506,7 @@ impl CacheLocker {
501506
gctx: &GlobalContext,
502507
mode: CacheLockMode,
503508
) -> CargoResult<Option<CacheLock<'_>>> {
504-
let mut state = self.state.borrow_mut();
509+
let mut state = self.state.lock().unwrap();
505510
if state.lock(gctx, mode, NonBlocking)? == LockAcquired {
506511
Ok(Some(CacheLock { mode, locker: self }))
507512
} else {
@@ -519,7 +524,7 @@ impl CacheLocker {
519524
/// `DownloadExclusive` will return true if a `MutateExclusive` lock is
520525
/// held since they overlap.
521526
pub fn is_locked(&self, mode: CacheLockMode) -> bool {
522-
let state = self.state.borrow();
527+
let state = self.state.lock().unwrap();
523528
match (
524529
mode,
525530
state.cache_lock.count,

0 commit comments

Comments
 (0)