Add spawn_in_current_thread option to run_process/open_process#3462
Closed
Vansh-Sharma27 wants to merge 1 commit into
Closed
Add spawn_in_current_thread option to run_process/open_process#3462Vansh-Sharma27 wants to merge 1 commit into
Vansh-Sharma27 wants to merge 1 commit into
Conversation
run_process and open_process spawn the child by calling Popen() inside trio.to_thread.run_sync, which actually runs on a cached worker thread, not the thread that called run_process. Python-level state like context vars gets copied over fine, but OS-level per-thread state (network namespace from setns, capabilities, CPU affinity) does not, so the child can end up inheriting whatever the worker thread's state happens to be instead of the caller's. That's the bug in python-trio#3360. Added an opt-in spawn_in_current_thread=True kwarg that calls Popen directly on the calling thread instead of dispatching to a worker thread, so the child actually gets the calling thread's OS state. This briefly blocks the event loop while Popen runs, so it's off by default and existing behavior is unchanged. Added a regression test that uses sched_getaffinity/sched_setaffinity to actually reproduce the bug on Linux (skipped elsewhere since those calls aren't available on macOS/Windows), plus an equivalence test checking run_process/open_process behave the same with the flag on or off other than which thread spawns the child.
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (93.93939%) is below the target coverage (100.00000%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #3462 +/- ##
====================================================
- Coverage 100.00000% 99.98972% -0.01028%
====================================================
Files 128 128
Lines 19417 19448 +31
Branches 1317 1320 +3
====================================================
+ Hits 19417 19446 +29
- Misses 0 1 +1
- Partials 0 1 +1
🚀 New features to boost your workflow:
|
Contributor
|
Blocking the event loop for this is unacceptable, sorry! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3360
Root cause
run_process/open_processcallsubprocess.Popen()viatrio.to_thread.run_sync. Trio'sto_thread.run_syncdoesn't spin upa fresh OS thread per call — it pulls an existing worker thread out of
THREAD_CACHE(src/trio/_core/_thread_cache.py) if one is idle, andonly falls back to creating a new one if the cache is empty
(
ThreadCache.start_thread_soon, which doesself._idle_workers.popitem()before deciding to spawn). That cache is what makes
to_thread.run_synccheap to call repeatedly, but it also means the thread that ends up
calling
Popen()for any givenrun_processcall is whatever threadhappened to finish its previous job and get parked in the cache. It is
not the thread that called
run_process, and it's not a fresh threadeither.
Python-level state (contextvars, etc.) gets propagated to that worker
thread correctly, which is why this is easy to miss. But OS-level
per-thread state is a kernel-level property of the specific thread,
not something Python copies around:
setns(2)to move a thread into adifferent Linux network namespace, thread-directed capabilities, and
CPU affinity (
pthreads(7)) all stick to whichever OS thread you setthem on. If you set one of those up on your own thread and then call
run_process, the child is actually forked from a leftover cachedworker thread that never had your namespace/capabilities/affinity
applied to it in the first place, so it silently inherits whatever
that worker thread's state happens to be instead. That's exactly the
network namespace problem the issue reporter (tik-stbuehler) ran into.
The issue thread also discusses spawning a dedicated thread per call
with
clone(2)so it can inherit the caller's namespace, buttik-stbuehler pushed back on that in the comments since it throws away
the whole point of having a thread cache (or requires a per-trio-loop
thread-local pool, which is a much bigger structural change and was
posted as a workaround, not a proposed fix). The issue itself asks for
two more modest things instead: document the behavior, and add a flag
to spawn from the current thread. This PR does both.
Fix
Added
spawn_in_current_thread: bool = Falseto bothrun_processand
open_process. WhenTrue,Popen()is called synchronously onthe calling thread instead of being handed off to
trio.to_thread.run_sync, so the child reliably gets the callingthread's OS-level state because it really is forked from that thread.
This obviously blocks the calling thread (and the event loop, if
called from Trio's main task) for as long as
Popen()takes, which isnormally fast but is still a real synchronous blocking call, so it's
opt-in and the default behavior/performance is completely unchanged.
Also added the
.. note::docs paragraph the issue asked for on bothrun_processandopen_process(and on theTYPE_CHECKINGstubsused for Windows / the Unix overloads, so docs stay consistent across
the type-stub split this module already has).
Testing
The namespace scenario from the issue needs root/
CAP_SYS_ADMINandreal network namespace setup, which isn't practical to run in CI. CPU
affinity is gated by the exact same bug mechanism (it's also per-thread
OS state that doesn't follow a thread cache hand-off) but is settable
by an unprivileged process via
os.sched_setaffinity, so I used thatas a stand-in to get an actual reproduction instead of just testing
that the option exists:
test_spawn_in_current_thread_affinity(Linux-only, skipped onmacOS/Windows since
sched_getaffinity/sched_setaffinityaren'tavailable there): runs one
run_processcall first to warm upTHREAD_CACHEwith a worker thread while affinity is unrestricted,then restricts the calling thread's affinity to a single CPU and
checks that a child spawned the default way still reports the old,
unrestricted mask (because it came from the stale cached worker
thread), while a child spawned with
spawn_in_current_thread=Truereports the restricted mask. This fails without the fix and passes
with it.
test_spawn_in_current_thread_equivalence: cross-platform sanitycheck that
run_process/open_processbehave the same with theflag on vs. off (stdin/stdout/return code), since the flag should
only change which thread calls
Popen(), nothing about theresulting process.
Unpack[...]kwarg to the Windows/UnixTYPE_CHECKINGoverloads and atype_tests/subprocesses.pyentry sothe new kwarg type-checks on every overload shape.
newsfragments/3360.feature.rst.Ran the project's full check suite locally before opening this:
pytest src(825 passed, 71 skipped, 2 xfailed — all pre-existing,unrelated to this change),
mypy --platform linux/darwin/win32,pyrighton bothtype_testsdirs,check_type_completeness.py,gen_exports.py --test, andpre-commit run --all-files. All green.