Skip to content

Commit b0b4d2f

Browse files
youknowoneyouknowone
authored
lock: add an opt-in rwlock that detaches before it blocks (#8556)
* lock: add an opt-in rwlock that detaches before it blocks A thread blocked acquiring a lock reaches no safepoint, so stop-the-world cannot stop it, and the lock it waits for is routinely one a thread the requester already suspended is holding. `RawDetachingRwLock` wraps the raw rwlock and hands the wait for a contended acquire to a hook that leaves the interpreter first; an acquire that takes the lock on its first try does not reach the hook. The vm installs the hook during interpreter init and implements it with `allow_threads`. The wait ends with the lock acquired while detached, so re-attaching can park the thread holding it. That is only safe where nothing reachable from a stop-the-world section takes the same lock, so it is opt-in per lock: `PyDetachingRwLock` is a separate type from `PyRwLock`, and `Traverse` is not implemented for it, so a payload holding one cannot derive `Traverse`. `PyByteArray::inner` takes it. `BorrowedValue`/`BorrowedValueMut` gain the matching mapped-guard variants. `a_thread_blocked_on_a_lock_does_not_stall_stop_the_world` blocks an interpreter thread on a `PyDetachingRwLock` and asserts stop-the-world still completes, running the stop on its own thread with a timeout so a stop that never completes fails rather than hangs. Without the hook installed it fails on the 10 s timeout; with it, it passes in 0.07 s. Assisted-by: Claude * lock: stop detaching where the lock may already be held `upgrade` runs with the upgradable lock held, and `lock_shared_recursive` may be the re-entrant take of a lock the calling thread holds; detaching there parks a thread holding the lock, which is what this type documents it must not do. They forward to the wrapped lock instead. `lock_upgradable` starts from holding nothing, but nothing takes an upgradable read of one of these, so it forwards too. `lock_shared` and `lock_exclusive` still detach. Also narrow two claims the comments overstated. Not implementing `Traverse` enforces the opt-in rule only against collections, not against the other things that stop the world. And the requester exemption the hook relies on is wider than `_PyEval_StopTheWorld` gives, so it is a local invariant. Assisted-by: Claude * os.readinto: read aside when the fd can wait `readinto` held the destination's write lock for the whole call, including the `read(2)` inside `allow_threads`. On a pipe, socket or terminal that read returns only when the other end writes, so a thread reaching the same object waited on a lock for an unbounded time, reaching no safepoint while it did. Take the fd that answers without waiting directly, as before, and otherwise read into scratch and take the lock only for the copy. This is what `FileIO.readinto`, `socket.recv_into` and `socket.recvfrom_into` already do; `os.readinto` was the site left over. The EINTR retry moves to `read_into_slice`, unchanged. Assisted-by: Claude * fcntl: detach around the calls that wait Every call in this module ran with the thread attached, so a thread inside one reached no safepoint until it returned. `flock(LOCK_EX)` and `lockf(F_LOCK)` return when whoever holds the lock gives it up, which may be never, and an ioctl on a terminal or socket answers when the device is ready to; the world could not be stopped for that long. `fcntl_fcntl_impl`, `fcntl_ioctl_impl`, `fcntl_flock_impl` and `fcntl_lockf_impl` all release around the call. `ioctl` with `mutate_flag` additionally held the target's write lock for the whole call, so a thread reaching the same object waited on a lock for as long as the device took. Its bytes now go in and come back through a buffer of our own, as `fcntl_ioctl_impl` copies through one of its own for anything up to IOCTL_BUFSZ. The export the argument holds is what keeps the length from changing in between. test_fcntl and test_ioctl pass. Assisted-by: Claude * openssl: read aside instead of holding the caller's buffer locked `SSLSocket.read` wrote straight into the destination buffer, holding the lock that reaching its bytes takes for the whole call. That read returns when the peer writes, which may be never, so a thread touching the same object waited on that lock for as long as the peer took, reaching no safepoint while it did. Read into a buffer of our own and take the destination's lock only for the copy. The rustls backend already reads this way, and `_ssl__SSLSocket_read_impl` works from a `Py_buffer` whose critical section ended before the read. `test_ssl` on this backend fails the same 16 tests before and after. Assisted-by: Claude * vm: add the inverse of allow_threads for callbacks A call that detaches hands the thread to stop-the-world, which counts it as parked. A callback that reaches Python from inside such a call would then run on a thread the requester believes is stopped, and nothing in the vm could stop it: `attach_thread` and `detach_thread` are private to `thread.rs`, and `allow_threads` only goes the one way. `attach_for_callback` attaches for the duration of the closure and returns the thread to where it was, the way `PyGILState_Ensure` and `PyGILState_Release` bracket `_servername_callback`. It tests for "not ATTACHED" rather than for DETACHED, so a thread a stop-the-world has already moved to SUSPENDED routes through `attach_thread` and parks there until the world starts again. `a_callback_inside_a_detached_call_waits_for_the_world` stops the world with a thread detached, then turns that thread loose at a callback and asserts it does not run until the world starts. With the transition disabled it fails. Assisted-by: Claude * openssl: attach around the callbacks that run Python `_servername_callback` and `_msg_callback` reach the interpreter from inside an SSL call -- they take a reference to the Python callback, build arguments and call it. That call is about to detach, and running Python from a detached thread runs it on a thread a stop-the-world requester counts as parked. `_servername_callback` opens with `PyGILState_Ensure()` for the same reason. Both now rejoin the interpreter for the duration of the callback and give the thread back afterwards. The reference to the callback moves inside that section, since taking it is itself an interpreter operation; the check for whether a callback is set at all stays outside, so a socket with none set never reaches the interpreter. No behavior change yet: nothing detaches around these calls, so `attach_for_callback` finds the thread already attached and just runs. Assisted-by: Claude * openssl: detach around the SSL calls that wait On a socket with a timeout the wait lands in `select` -> `sock_wait`, which already detaches. On a blocking socket there is no such return: `SSL_read` blocks in `recv(2)` through `impl Read for &PySocket`, with the thread attached and the connection's write lock held, so the world could not be stopped for as long as the peer stayed silent. `SSL_do_handshake`, `SSL_read_ex`, `SSL_write_ex` and `SSL_shutdown` all run between `Py_BEGIN_ALLOW_THREADS` and `Py_END_ALLOW_THREADS`; these now do too, in both socket and BIO mode as there. The connection lock stays held across the call and so becomes a detaching lock: a thread reaching the same socket gives up its interpreter rather than wait for it attached. `connection` is `#[pytraverse(skip)]`, so a collection does not walk into it and never takes that lock -- which is the rule for opting in, though the skip is what supplies it here rather than the missing `Traverse`. A server that completes a handshake and then says nothing used to deadlock the whole process: the collector suspended the main thread at a safepoint and then waited forever for the reader, so even the test's own timeout could not fire. It now collects in 3 ms. Verified separately that a Python `_msg_callback` still runs from inside the handshake -- 920 invocations across 40 handshakes with a collector looping, five runs clean. test_ssl on this backend fails the same 8 tests before and after, by name, and openssl.rs draws no clippy warning it did not draw before. Assisted-by: Claude * lock: assert the opt-in rule against every stop-the-world section Not implementing `Traverse` for `PyDetachingRwLock` states the rule to a collection: a payload holding one cannot derive `Traverse`, so a collection cannot walk into it. It says nothing to the other sections that stop the world -- fork, traceback dumps, frame enumeration -- and `#[pytraverse(skip)]` steps around it besides, which is how `_SSLSocket.connection` holds one. For those the rule was a comment. `set_world_stopped` records, on the one thread still running inside a stopped world, that it is that thread; `lock_shared` and `lock_exclusive` assert it is not set. A section that took one of these could block on a lock a parked thread holds and only that section can release, which is the deadlock the rule exists to prevent. Debug builds only; release builds track nothing. Nothing in the tree trips it: 180 rounds of collect, `sys._current_frames`, `faulthandler.dump_traceback` and 18 forks with four threads churning bytearrays, plus test_gc/test_bytes/test_threading/test_memoryview/test_buffer on a debug build, all clean. `taking_one_while_stopping_the_world_is_caught` takes one with the flag set and asserts the panic, so the guard is not dead code. Assisted-by: Claude * clippy: drop std_instead_of_core suppressions 1.98 no longer reports `std::io` items for the lint, so the six `expect` attributes for it are unfulfilled. Also drops `from_iter_instead_of_collect` from the workspace lint table, which 1.98 removed. Assisted-by: Claude * lock: spell out why the detaching rwlock exists States the motivation as the three-thread cycle it avoids: how a stop reaches a DETACHED thread but not an ATTACHED one, why a thread blocked on a lock reaches no safepoint, and why the waiter rather than the holder gives way. Puts it on `RawDetachingRwLock`, which is public and so rendered; the module doc is private and keeps only the hook description. Assisted-by: Claude * fcntl: raise the error the call returned The six detached calls discarded the `io::Error` and read `errno` again after `allow_threads`, which re-attaches in between and can park on the way. `lockf` already converts the returned error; these now do too. Assisted-by: Claude * openssl: allocate the read buffer through the vm Without a `buffer` argument `read_len` is whatever non-negative size the caller passed, so `vec![0u8; read_len]` aborts on allocation failure. `new_zeroed_bytes` raises `MemoryError` instead, as the two other reads in this file already do. Assisted-by: Claude * lock: leave no attached blocking acquire on a detaching lock `lock_upgradable` starts from holding nothing, so it detaches like `lock_shared`. A recursive read cannot: it may be the re-entrant take of a lock this thread holds, so detaching there parks a thread holding it, and staying attached stalls stop-the-world. `RawRwLockRecursive` is no longer implemented, which removes `read_recursive` from these locks; nothing took one. The assertion test no longer replaces the panic hook, which is process-wide and was suppressing panic output from whatever else ran beside it. Assisted-by: Claude * vm: wait for the actual block in the stop-the-world test The worker signalled before `read()` and the test slept 50ms, so a stop could complete with nothing blocked on the lock and the test would pass having checked nothing. It now publishes its thread id from inside the interpreter and the test waits for that slot to reach DETACHED, bounded so an acquire that never detaches fails rather than hangs. Assisted-by: Claude
1 parent 7931cb0 commit b0b4d2f

11 files changed

Lines changed: 937 additions & 192 deletions

File tree

‎Cargo.toml‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -398,7 +398,6 @@ explicit_iter_loop = "warn"
398398
filter_map_next = "warn"
399399
flat_map_option = "warn"
400400
format_collect = "warn"
401-
from_iter_instead_of_collect = "warn"
402401
inconsistent_struct_constructor = "warn"
403402
index_refutable_slice = "warn"
404403
inefficient_to_string = "warn"

‎crates/common/src/borrow.rs‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
use crate::lock::{
2-
MapImmutable, PyImmutableMappedMutexGuard, PyMappedMutexGuard, PyMappedRwLockReadGuard,
2+
MapImmutable, PyImmutableMappedMutexGuard, PyMappedDetachingRwLockReadGuard,
3+
PyMappedDetachingRwLockWriteGuard, PyMappedMutexGuard, PyMappedRwLockReadGuard,
34
PyMappedRwLockWriteGuard, PyMutexGuard, PyRwLockReadGuard, PyRwLockWriteGuard,
45
};
56
use alloc::fmt;
@@ -24,13 +25,15 @@ pub enum BorrowedValue<'a, T: ?Sized> {
2425
MappedMuLock(PyImmutableMappedMutexGuard<'a, T>),
2526
ReadLock(PyRwLockReadGuard<'a, T>),
2627
MappedReadLock(PyMappedRwLockReadGuard<'a, T>),
28+
MappedDetachingReadLock(PyMappedDetachingRwLockReadGuard<'a, T>),
2729
}
2830
impl_from!('a, T, BorrowedValue<'a, T>,
2931
Ref(&'a T),
3032
MuLock(PyMutexGuard<'a, T>),
3133
MappedMuLock(PyImmutableMappedMutexGuard<'a, T>),
3234
ReadLock(PyRwLockReadGuard<'a, T>),
3335
MappedReadLock(PyMappedRwLockReadGuard<'a, T>),
36+
MappedDetachingReadLock(PyMappedDetachingRwLockReadGuard<'a, T>),
3437
);
3538

3639
impl<'a, T: ?Sized> BorrowedValue<'a, T> {
@@ -59,6 +62,9 @@ impl<'a, T: ?Sized> BorrowedValue<'a, T> {
5962
Self::MappedReadLock(m) => {
6063
BorrowedValue::MappedReadLock(PyMappedRwLockReadGuard::map(m, f))
6164
}
65+
Self::MappedDetachingReadLock(m) => {
66+
BorrowedValue::MappedDetachingReadLock(PyMappedDetachingRwLockReadGuard::map(m, f))
67+
}
6268
}
6369
}
6470
}
@@ -73,6 +79,7 @@ impl<T: ?Sized> Deref for BorrowedValue<'_, T> {
7379
Self::MappedMuLock(m) => m,
7480
Self::ReadLock(r) => r,
7581
Self::MappedReadLock(m) => m,
82+
Self::MappedDetachingReadLock(m) => m,
7683
}
7784
}
7885
}
@@ -90,6 +97,7 @@ pub enum BorrowedValueMut<'a, T: ?Sized> {
9097
MappedMuLock(PyMappedMutexGuard<'a, T>),
9198
WriteLock(PyRwLockWriteGuard<'a, T>),
9299
MappedWriteLock(PyMappedRwLockWriteGuard<'a, T>),
100+
MappedDetachingWriteLock(PyMappedDetachingRwLockWriteGuard<'a, T>),
93101
}
94102

95103
impl_from!('a, T, BorrowedValueMut<'a, T>,
@@ -98,6 +106,7 @@ impl_from!('a, T, BorrowedValueMut<'a, T>,
98106
MappedMuLock(PyMappedMutexGuard<'a, T>),
99107
WriteLock(PyRwLockWriteGuard<'a, T>),
100108
MappedWriteLock(PyMappedRwLockWriteGuard<'a, T>),
109+
MappedDetachingWriteLock(PyMappedDetachingRwLockWriteGuard<'a, T>),
101110
);
102111

103112
impl<'a, T: ?Sized> BorrowedValueMut<'a, T> {
@@ -113,6 +122,9 @@ impl<'a, T: ?Sized> BorrowedValueMut<'a, T> {
113122
Self::MappedWriteLock(m) => {
114123
BorrowedValueMut::MappedWriteLock(PyMappedRwLockWriteGuard::map(m, f))
115124
}
125+
Self::MappedDetachingWriteLock(m) => BorrowedValueMut::MappedDetachingWriteLock(
126+
PyMappedDetachingRwLockWriteGuard::map(m, f),
127+
),
116128
}
117129
}
118130
}
@@ -127,6 +139,7 @@ impl<T: ?Sized> Deref for BorrowedValueMut<'_, T> {
127139
Self::MappedMuLock(m) => m,
128140
Self::WriteLock(w) => w,
129141
Self::MappedWriteLock(w) => w,
142+
Self::MappedDetachingWriteLock(w) => w,
130143
}
131144
}
132145
}
@@ -139,6 +152,7 @@ impl<T: ?Sized> DerefMut for BorrowedValueMut<'_, T> {
139152
Self::MappedMuLock(m) => &mut *m,
140153
Self::WriteLock(w) => &mut *w,
141154
Self::MappedWriteLock(w) => &mut *w,
155+
Self::MappedDetachingWriteLock(w) => &mut *w,
142156
}
143157
}
144158
}

‎crates/common/src/lock.rs‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ use lock_api::{
88

99
cfg_select! {
1010
feature = "threading" => {
11+
pub use detaching::{BlockingWaitHook, set_blocking_wait_hook, set_world_stopped};
1112
pub use parking_lot::{RawMutex, RawRwLock, RawThreadId};
1213
pub use std::sync::OnceLock as OnceCell;
1314
pub use core::cell::LazyCell;
@@ -47,6 +48,8 @@ cfg_select! {
4748
}
4849
}
4950

51+
mod detaching;
52+
pub use detaching::RawDetachingRwLock;
5053
mod immutable_mutex;
5154
pub use immutable_mutex::*;
5255
mod thread_mutex;
@@ -60,6 +63,19 @@ pub type PyThreadMutex<T> = ThreadMutex<RawMutex, RawThreadId, T>;
6063
pub type PyThreadMutexGuard<'a, T> = ThreadMutexGuard<'a, RawMutex, RawThreadId, T>;
6164
pub type PyMappedThreadMutexGuard<'a, T> = MappedThreadMutexGuard<'a, RawMutex, RawThreadId, T>;
6265

66+
/// A `PyRwLock` for data a thread may hold locked across a blocking call.
67+
///
68+
/// Waiting for one of these leaves the interpreter first, so a thread blocked
69+
/// on it is a thread stop-the-world can park. That is only safe where a
70+
/// collection never takes the same lock — see [`RawDetachingRwLock`] — so this
71+
/// is opt-in per lock rather than what every `PyRwLock` does.
72+
pub type PyDetachingRwLock<T> = RwLock<RawDetachingRwLock, T>;
73+
pub type PyDetachingRwLockReadGuard<'a, T> = RwLockReadGuard<'a, RawDetachingRwLock, T>;
74+
pub type PyDetachingRwLockWriteGuard<'a, T> = RwLockWriteGuard<'a, RawDetachingRwLock, T>;
75+
pub type PyMappedDetachingRwLockReadGuard<'a, T> = MappedRwLockReadGuard<'a, RawDetachingRwLock, T>;
76+
pub type PyMappedDetachingRwLockWriteGuard<'a, T> =
77+
MappedRwLockWriteGuard<'a, RawDetachingRwLock, T>;
78+
6379
pub type PyRwLock<T> = RwLock<RawRwLock, T>;
6480
pub type PyRwLockUpgradableReadGuard<'a, T> = RwLockUpgradableReadGuard<'a, RawRwLock, T>;
6581
pub type PyRwLockReadGuard<'a, T> = RwLockReadGuard<'a, RawRwLock, T>;

0 commit comments

Comments
 (0)