shahar1 commented on code in PR #74780:
URL: https://github.com/apache/airflow/pull/74780#discussion_r4306791560
##########
providers/standard/src/airflow/providers/standard/hooks/subprocess.py:
##########
@@ -93,12 +93,18 @@ def run_command(
line = ""
if self.sub_process is None:
raise RuntimeError("The subprocess should be created here and
is None!")
- if self.sub_process.stdout is not None:
- for raw_line in iter(self.sub_process.stdout.readline, b""):
- line = raw_line.decode(output_encoding,
errors="backslashreplace").rstrip()
- self.log.info("%s", line)
-
- self.sub_process.wait()
+ # Not `with Popen(...)`: its __exit__ blocks in wait(), so on an
execution_timeout
+ # the task's on_kill() would never run to SIGTERM the child.
+ try:
+ if self.sub_process.stdout is not None:
+ for raw_line in iter(self.sub_process.stdout.readline,
b""):
+ line = raw_line.decode(output_encoding,
errors="backslashreplace").rstrip()
+ self.log.info("%s", line)
+
+ self.sub_process.wait()
+ finally:
+ if self.sub_process.stdout is not None:
Review Comment:
**major** — Closing the pipe in `finally` also runs on the
`execution_timeout` path, while the child is still alive and *before*
`on_kill()` sends SIGTERM. The child was started with `restore_signals=True`,
so SIGPIPE is at its default action: the next time it writes to stdout it is
killed by SIGPIPE. A command whose SIGTERM handler writes anything (a very
common `trap 'echo cleaning up; …' TERM` pattern) is now killed in the middle
of its cleanup.
I reproduced it with the real hook, a `SIGALRM` that raises a
`BaseException` inside `run_command` (as `AirflowTaskTimeout` does), then
`hook.send_sigterm()` as `on_kill()` does:
```bash
trap 'echo cleaning up; touch $MARKER; exit 1' TERM; echo started; while
true; do sleep 0.1; done
```
| hook | return code | cleanup ran |
|---|---|---|
| `main` | `1` | yes (3/3) |
| this PR | `-13` (SIGPIPE) | no (3/3) |
| this PR, close guarded by `poll() is not None` | `1` | yes (3/3) |
So the change defeats the guarantee the new code comment (and the PR body)
gives for not using `with Popen(...)`: the child is no longer reliably
terminated by `on_kill()`'s SIGTERM. Closing only once the child has exited
keeps the fix for the normal and read-error paths (the pipe is still closed
after a normal run, and the mocked read-error test still passes because
`poll()` returns a mock), and leaves the timeout path as it is on `main`, where
the hook is torn down shortly after anyway.
A regression test for this path would be good: run a `trap … TERM` command,
interrupt `run_command` with a `BaseException`, call `send_sigterm()`, and
assert the cleanup ran.
> **Flag any changed or added behaviour without a corresponding test, and
flag tests that a reviewer cannot fail by reverting the PR's change.** The
target is exactly 100% coverage of what the PR changes — no more, no less.
Tests for pre-existing logic or standard-library/third-party functions are
padding; missing tests for changed behaviour are gaps. Exception: deliberate
behaviour or integration tests may cross those boundaries by design.
>
> —
[.github/instructions/code-review.instructions.md](https://github.com/apache/airflow/blob/main/.github/instructions/code-review.instructions.md#testing-requirements)
--
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]
