Skip to content

Commit

Permalink
Fix a deadlock involve scheduler.all_digests, and add a note. (#11723)
Browse files Browse the repository at this point in the history
### Problem

By chance, a garbage collection thread started up and called `lease_files_in_graph` while I happened to be running Pants in the foreground. This caused a deadlock because `lease_files_in_graph` was not releasing the GIL before touching the `Graph` (via the `Scheduler`)... and foreground interactions with the `Graph` frequently need to acquire the GIL to check for equality.

### Solution

Move the problematic call to `scheduler.all_digests()` under `allow_threads` and add a note. Additionally, move one other potentially-problematic method call under `allow_threads`.

### Result

A rare (hopefully?) deadlock is prevented. See #11722 for how we might make this less likely in the future.

[ci skip-build-wheels]
  • Loading branch information
stuhood committed Mar 17, 2021
1 parent 10c6f45 commit 4e53dbd
Showing 1 changed file with 9 additions and 3 deletions.
12 changes: 9 additions & 3 deletions src/rust/engine/src/externs/interface.rs
Expand Up @@ -1314,8 +1314,8 @@ fn graph_visualize(
with_scheduler(py, scheduler_ptr, |scheduler| {
with_session(py, session_ptr, |session| {
let path = PathBuf::from(path);
scheduler
.visualize(session, path.as_path())
// NB: See the note on with_scheduler re: allow_threads.
py.allow_threads(|| scheduler.visualize(session, path.as_path()))
.map_err(|e| {
let e = format!("Failed to visualize to {}: {:?}", path.display(), e);
PyErr::new::<exc::Exception, _>(py, (e,))
Expand Down Expand Up @@ -1469,8 +1469,9 @@ fn lease_files_in_graph(
) -> PyUnitResult {
with_scheduler(py, scheduler_ptr, |scheduler| {
with_session(py, session_ptr, |session| {
let digests = scheduler.all_digests(session);
// NB: See the note on with_scheduler re: allow_threads.
py.allow_threads(|| {
let digests = scheduler.all_digests(session);
scheduler
.core
.executor
Expand Down Expand Up @@ -1885,6 +1886,11 @@ where
/// context methods provide immutable references. The remaining types are not intended to be shared
/// between threads, so mutable access is provided.
///
/// TODO: The `Scheduler` and `Session` objects both have lots of internal locks: in general, the GIL
/// should be released (using `py.allow_thread(|| ..)`) before any non-trivial interactions with
/// them. In particular: methods that use the `Graph` should be called outside the GIL. We should
/// make this less error prone: see https://github.com/pantsbuild/pants/issues/11722.
///
fn with_scheduler<F, T>(py: Python, scheduler_ptr: PyScheduler, f: F) -> T
where
F: FnOnce(&Scheduler) -> T,
Expand Down

0 comments on commit 4e53dbd

Please sign in to comment.