Conversation
| :param use_exec: If True, start a fresh Python interpreter via ``os.posix_spawn`` | ||
| instead of a bare ``os.fork``: forced on platforms that need it (macOS, whose | ||
| Objective-C frameworks are not fork-safe) and opted into for the task process | ||
| elsewhere via ``[core] execute_tasks_new_python_interpreter``. Unlike |
There was a problem hiding this comment.
Nit/observation: this should have been in the worker section. Oh well, (separate issue)
| assert "setpgroup" not in spawn.call_args.kwargs | ||
|
|
||
| @pytest.mark.skipif(sys.platform == "win32", reason="os.fork/os.register_at_fork are POSIX-only") | ||
| def test_hanging_after_fork_handler_wedges_bare_fork_but_not_posix_spawn(self): |
There was a problem hiding this comment.
This is probably the only test worth keeping
| instead of a bare ``os.fork``: forced on platforms that need it (macOS, whose | ||
| Objective-C frameworks are not fork-safe) and opted into for the task process | ||
| elsewhere via ``[core] execute_tasks_new_python_interpreter``. Unlike | ||
| ``fork()`` followed by ``execv()``, ``posix_spawn`` never runs |
There was a problem hiding this comment.
The module docstring above (_FORK_EXEC_PLATFORMS, around line 498) and the _child_exec_main / _task_process_uses_exec docstrings still describe the old mechanism: "Calling os.execv immediately after os.fork" and "os.set_inheritable clears FD_CLOEXEC on those FDs so they survive the upcoming exec". There is no set_inheritable call on this path any more (the dup2 file actions clear it), so that sentence is now wrong rather than just dated. Worth updating those to match this one.
| if use_exec: | ||
| # file_actions run as part of the spawn itself -- no forked child to run | ||
| # imperative dup2 code in. | ||
| file_actions = [ |
There was a problem hiding this comment.
The removed block explained why this dup2 ordering is safe: all four source fds are >= 3 because 0/1/2 are open in every launch path, so no dup2 onto 0..3 clobbers a source that hasn't been placed yet. The file actions run in the same order and rely on the same invariant, and the set_inheritable backstop for a same-fd dup2 is gone too. I'd keep that sentence here, since nothing else in the code now says it.
execute_tasks_new_python_interpreter (apache#72164) and the macOS-forced exec path both still call os.fork() before execv(), and CPython's os.fork() runs every os.register_at_fork(after_in_child=...) callback synchronously inside the fork() call itself, before any Python-level code -- including the planned execv() -- gets control back. A third-party library's own fork handler that blocks there (confirmed live: a customer's task process hung inside datadog's dogstatsd client, which registers such a handler by default) hangs the child before exec is ever reached, regardless of how soon the caller tries to exec. This isn't just Airflow's own known OpenSSL provider-store case (apache#71707) -- it's any library that registers an at-fork handler that isn't async-signal-safe, which fork+exec cannot protect against structurally, no matter how the call sites are ordered. os.posix_spawn() doesn't have this gap: CPython's binding never calls PyOS_AfterFork_Child(), and glibc's own posix_spawn (2.24+) uses clone(CLONE_VM|CLONE_VFORK) rather than fork(), so registered os.register_at_fork()/pthread_atfork() handlers are structurally unreachable, not just less likely to hang. It's also not a new cost on top of the existing exec path -- benchmarked against a real Airflow import, posix_spawn is measurably not more expensive than the fork+exec it replaces (slightly cheaper, from skipping fork()'s own copy-on-write setup before the exec). No new config surface: wherever use_exec was already True (the platform gate or execute_tasks_new_python_interpreter), the spawn mechanism underneath is now always posix_spawn. That decision was already made by existing config; this only changes how "give me a fresh interpreter" is implemented once it's been decided, matching the existing dup2/env/ process-group semantics via posix_spawn's file_actions/env/setpgroup parameters instead of imperative code in a forked child.
7ff323a to
1b987ba
Compare
3 participants
execute_tasks_new_python_interpreter(#72164) and the macOS-forced exec path both still callos.fork()beforeexecv(). CPython'sos.fork()runs everyos.register_at_fork(after_in_child=...)callback synchronously, inside thefork()call itself, before any Python-level code — including the plannedexecv()— gets control back. A third-party library's own fork handler that blocks there hangs the child before exec is ever reached, regardless of how soon the caller tries to exec — confirmed live: a deployment's task process hung insidedatadog's dogstatsd client, which registers exactly such a handler by default. This isn't just Airflow's own known OpenSSL provider-store case (#71707) — it's any library that registers an at-fork handler that isn't async-signal-safe, and fork+exec can't protect against that structurally, no matter how the call sites are ordered.os.posix_spawn()doesn't have this gap: CPython's binding never callsPyOS_AfterFork_Child(), and glibc's ownposix_spawn(2.24+) usesclone(CLONE_VM|CLONE_VFORK)rather thanfork(), so registeredos.register_at_fork()/pthread_atfork()handlers are structurally unreachable, not just less likely to hang. It's also not a new cost on top of the existing exec path — benchmarked against a real Airflow import,posix_spawnis measurably not more expensive than the fork+exec it replaces (slightly cheaper, from skippingfork()'s own copy-on-write setup before the exec).No new config surface: wherever
use_execwas alreadyTrue(the platform gate orexecute_tasks_new_python_interpreter), the spawn mechanism underneath is now alwaysposix_spawn. That decision is already made by existing config; this only changes how "give me a fresh interpreter" is implemented once it's been decided.related: #71707, #72164, #72493
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Sonnet 5 following the guidelines