Fix potential deadlock in CacheState::lock

[smoelius]

This PR fixes a potential source of deadlock in the CacheState::lock function (here), as explained below.

I ran into this deadlock while testing Dylint. For example, in this GitHub run, two jobs were killed after running for six hours. This fix seems to resolve the deadlock (e.g., see this run, which uses the fix).

Until this fix (or a similar one) appears in rustup-installable Cargo, is there an easy workaround?

---

A CacheState struct holds two recursive locks: mutate_lock and cache_lock.

When MutateExclusive is passed to CacheState::lock, it tries to acquire both locks. First, it tries to acquire mutate_lock, then it tries to acquire cache_lock.

The problematic case is when it acquires the first, but not the second.

Note that if the second cannot be acquired because of an error, the mutate_lock recursive lock is decremented:

cargo/src/cargo/util/cache_lock.rs

Lines 412 to 415 in 84709f0

	Err(e) => {
	self.mutate_lock.decrement();
	return Err(e);
	}

However, if the second would simply block, LockingResult::WouldBlock is returned.

CacheState::lock is called from two places. One of those locations is in CacheLocker::try_lock:¹

cargo/src/cargo/util/cache_lock.rs

Lines 502 to 506 in 84709f0

	if state.lock(gctx, mode, NonBlocking)? == LockAcquired {
	Ok(Some(CacheLock { mode, locker: self }))
	} else {
	Ok(None)
	}

Note that CacheLocker::try_lock creates a CacheLock if and only if LockingResult::LockAcquired is returned.

Furthermore, when a CacheLock is dropped, it decrements both mutate_lock and cache_lock:

cargo/src/cargo/util/cache_lock.rs

Lines 443 to 446 in 84709f0

	MutateExclusive => {
	state.cache_lock.decrement();
	state.mutate_lock.decrement();
	}

A scan of cache_lock.rs shows that there are only three places² where mutate_lock.decrement() is called: the error location in CacheState::lock (referenced above), and two places in CacheLock::drop.

Thus, if LockingResult::WouldBlock is returned from CacheState::lock, mutate_lock is never decremented.

Footnotes

1. The other location is in CacheLocker::lock, which calls CacheState::lock with BlockingMode::Blocking. For that reason, CacheLocker::lock should not return WouldBlock when called from this location. ↩

2. https://github.com/rust-lang/cargo/blob/84709f085062cbf3c51fa507527c1b2334015178/src/cargo/util/cache_lock.rs#L413, https://github.com/rust-lang/cargo/blob/84709f085062cbf3c51fa507527c1b2334015178/src/cargo/util/cache_lock.rs#L438, and https://github.com/rust-lang/cargo/blob/84709f085062cbf3c51fa507527c1b2334015178/src/cargo/util/cache_lock.rs#L445 ↩

[rustbot]

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[epage]

Thanks!

[smoelius]

Is there any chance this fix could make it into Thursday's release? (Sorry if this is a crazy or unreasonable ask.)

[epage]

Despite the forge saying 1.89 beta will be branched on the 20th, it doesn't look like its been created yet, so this should at least make that without a beta backport.

We have 3 days before 1.88 is released. We just did a submodule update for rust-lang/rust (rust-lang/rust#142898). So to do this we'd need to feel comfortable enough with this PR with little testing, get a beta backport merged, and update the submodule. I'm not even sure what the exact cut off for a change like this is. This problem was introduced in #12706 which was released in 1.75. I feel like we can live for 6 more weeks with this but I'll bring it up with the team.

[smoelius]

Thanks very much for the explanation.

[epage]

Looks like we did miss the 1.89 window so I backported this in #15699.

[smoelius]

Thanks for keeping me informed, and thanks for going to all of this trouble. 🙏