Dev-iL commented on code in PR #74780:
URL: https://github.com/apache/airflow/pull/74780#discussion_r4306994587
##########
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:
Confirmed, and thank you for the reproduction! Added
`test_run_command_keeps_stdout_open_for_sigterm_cleanup` for the path you
found. It asserts the marker exists and the child exits `1` rather than
`-SIGPIPE`, fails on the previous commit, and passes on this one.
The reason I did not use `poll() is not None` is the error path. On a real
`readline` failure the child is still running, so `poll()` returns `None` and
the pipe is never closed. It would also leave
`test_run_command_closes_stdout_pipe_on_read_error` green only because
`MagicMock.poll()` is truthy, as you noted yourself; a test that can't fail is
worse than no test.
Leaving this thread open for you to close since the fix isn't the one you
asked for.
--
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]
