Rebuild the Python runtime after fork - #9376
Conversation
`fork(2)` copies only the calling thread, so a forked child inherits a `CurrentThreadRuntime` whose worker threads no longer exist: the executor's sleeper list and the pool's handle list describe phantom threads, so `set_workers` believes it already has enough workers and spawns none, and every Vortex operation in the child blocks forever. The same applies to the process-global `blocking` pool behind `spawn_blocking`, whose inherited `idle_count` keeps it from growing. Tag the runtime with its owning pid and build a fresh one on first use from a different process, rather than trying to repair the inherited state; the stale state is leaked deliberately, since running its destructors would take locks that may not have survived the fork. An `os.register_at_fork(after_in_child=)` handler does the rebuild on the child's single-threaded startup path and warms the blocking pool. The shared session is repointed at the new executor, and the requested worker count is held outside the runtime so a child inherits it. `VortexFile` gains `path` and `__reduce__`, reopening by path in the receiving process, and the `datasets` filter path no longer needs its in-memory fallback now that a filter can be pickled into `num_proc` workers. Signed-off-by: Robert Kruszewski <github@robertk.io>
Merging this PR will degrade performance by 5.65%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
| } | ||
| } | ||
|
|
||
| static RUNTIME_STATE: RwLock<Option<RuntimeState>> = RwLock::new(None); |
There was a problem hiding this comment.
should this be thread local?
onursatici
left a comment
onursatici
left a comment
There was a problem hiding this comment.
I like the idea but I think it is still possible to be blocked.
In general for this to work we should:
- get the current_pid == state_pid check done without holding any locks, because that very lock could be stuck locked in fork
- when re-initialising the runtime state, we should make sure we are creating everything new, currently the internal Executor from the current thread runtime is copied and has an inner lock
| /// Force the process-global blocking-IO thread pool to spawn at least one live thread. | ||
| /// | ||
| /// Vortex reads route through `Handle::spawn_blocking`, which is backed by the `blocking` crate's | ||
| /// process-global pool. That pool only grows while `queue.len() > idle_count * 5`, and a forked |
There was a problem hiding this comment.
I think the condition is:
while inner.queue().len() > inner.idle_count * 5
&& inner.thread_count < thread_limit
which make this flaky, if thread_count was at the limit at the time of the fork, there is no way we can make this executor create new threads because there is no threads in the fork that would reduce the thread_count number when they die, so it is stuck
| })?; | ||
|
|
||
| // `fork(2)` leaves the inherited Vortex runtime unusable, so rebuild it in the child. | ||
| register_at_fork(py)?; |
There was a problem hiding this comment.
should we gate this with unix? I think having this unconditional here makes vortex not usable on windows
| } | ||
| } | ||
|
|
||
| static RUNTIME_STATE: RwLock<Option<RuntimeState>> = RwLock::new(None); |
There was a problem hiding this comment.
this is still prone to be forked while locked right? I can hold the read lock, get forked while holding it, in the new process there is no more readers but the read lock is held so it is not possible to get the write lock again
| /// The process that built this runtime. | ||
| pid: u32, | ||
| runtime: CurrentThreadRuntime, | ||
| pool: CurrentThreadWorkerPool, |
There was a problem hiding this comment.
so this pool has a reference to the Executor cloned from the runtime. This executor also has a lock inside. So even if we could make runtime state lock free, I think as long as we have a process global runtime, we share this same Executor everywhere. So if we fork while the executor is locked then the child process has no way to unlock
3 participants
Make python runtime pool fork friendly. While it's preferable to use threads
instead of processes we should gracefully recover from being forked instead of
deadlocking
Stack created with GitHub Stacks CLI • Give Feedback 💬