gang-zh commented on code in PR #72164:
URL: https://github.com/apache/airflow/pull/72164#discussion_r3971124958
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -513,8 +513,18 @@ def exit(n: int) -> NoReturn:
def _should_use_exec() -> bool:
- """Whether forked children should ``exec`` a fresh interpreter on this
platform."""
- return sys.platform in _FORK_EXEC_PLATFORMS
+ """
+ Whether forked children should ``exec`` a fresh interpreter.
+
+ Always on for platforms where bare fork is unsafe (macOS). Elsewhere it
can be
+ opted into with ``[core] execute_tasks_new_python_interpreter``: exec
replaces
+ the child's address space, so it cannot inherit a lock a supervisor thread
held
+ at fork time (e.g. OpenSSL's, which otherwise deadlocks the task at its
first
+ TLS call — see #71707).
+ """
+ if sys.platform in _FORK_EXEC_PLATFORMS:
+ return True
+ return conf.getboolean("core", "execute_tasks_new_python_interpreter",
fallback=False)
Review Comment:
Good catch — fixed in 9711663: `_child_exec_main()` now calls
`_make_process_nondumpable()` first, both security docs updated, and a
Linux-only `test_exec_child_reapplies_nondumpable` fork+execs and asserts the
flag goes 1 → 0.
##########
task-sdk/src/airflow/sdk/execution_time/supervisor.py:
##########
@@ -513,8 +513,18 @@ def exit(n: int) -> NoReturn:
def _should_use_exec() -> bool:
- """Whether forked children should ``exec`` a fresh interpreter on this
platform."""
- return sys.platform in _FORK_EXEC_PLATFORMS
+ """
+ Whether forked children should ``exec`` a fresh interpreter.
Review Comment:
Agreed — tasks only. 0fe8931 keeps `_should_use_exec()` as the platform gate
and reads the option in `ActivitySubprocess.start()` via
`_task_process_uses_exec()`; Dag processor and triggerer are unchanged. Same
shape a v3-2/v3-3 backport can carry.
##########
airflow-core/src/airflow/config_templates/config.yml:
##########
@@ -223,6 +223,10 @@ core:
* ``False``: Execute via forking of the parent process
* ``True``: Spawning a new python process, slower than fork, but means
plugin changes picked
up by tasks straight away
+
+ On workers this also makes the task process ``exec`` a fresh
interpreter right after the
Review Comment:
Done in 0fe8931: the description now says tasks-only, that the option was a
no-op for tasks on 3.x until now, and notes the Edge worker's second
interpreter; added `72164.significant.rst`.
##########
task-sdk/tests/task_sdk/execution_time/test_supervisor.py:
##########
@@ -4479,6 +4479,31 @@ def
test_api_client_clears_dag_bag_override_when_dag_is_none():
in_process_api_server.cache_clear()
+class TestShouldUseExec:
+ """The config opt-in for fork+exec on platforms where it is not forced
on."""
+
+ @pytest.mark.parametrize(
+ ("platform", "config_value", "expected"),
+ [
+ ("darwin", None, True),
+ ("darwin", "False", True),
+ ("linux", None, False),
+ ("linux", "False", False),
+ ("linux", "True", True),
+ ],
+ )
+ def test_should_use_exec(self, monkeypatch, platform, config_value,
expected):
+ monkeypatch.setattr(supervisor.sys, "platform", platform)
+ # The supervisor reads the task-sdk conf; the env var reaches it
regardless
Review Comment:
You're right — `conf_vars` covers the SDK conf; my PR-body rationale was
wrong and I've corrected it. b653700 switches the test to `conf_vars`.
--
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]
