kaxil commented on code in PR #73503:
URL: https://github.com/apache/airflow/pull/73503#discussion_r4066386526
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -755,80 +756,79 @@ def start(
# Place for child to send requests/read responses, and the server side
to read/respond
child_requests, read_requests = socketpair()
- # Open the socketpair before forking off the child, so that it is open
when we fork.
+ # Open the socketpair before starting the child, so that it is open
when we do.
child_logs, read_logs = socketpair()
- pid = os.fork()
- if pid == 0:
+ if use_exec:
+ # file_actions run as part of the spawn itself -- no forked child
to run
+ # imperative dup2 code in.
+ file_actions = [
Review 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.
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -729,12 +729,13 @@ def start(
"""
Fork and start a new subprocess with the specified target function.
- :param use_exec: If True, immediately ``os.execv`` a fresh Python
interpreter
- after ``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`` (a lock a
supervisor thread
- held at fork time cannot survive into a fresh address space).
- ``target`` is rehydrated in the exec'd child from its
``module:qualname``,
+ :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
+ ``fork()`` followed by ``execv()``, ``posix_spawn`` never runs
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
