Don't read from stdout/stderr on ES startup #879
Conversation
With this commit we avoid reading from stdout/stderr which might have been closed by the application and rely solely on the process' return code. Relates elastic/elasticsearch#50259
danielmitterdorfer
added
enhancement
|
@elasticmachine test this please |
| def _run_subprocess(command_line, env): | ||
| command_line_args = shlex.split(command_line) | ||
|
|
||
| # pylint: disable=subprocess-popen-preexec-fn |
There was a problem hiding this comment.
I know we didn't comment this last time when this was added, but when going back over this code I completely forgot about why we have a preexec_fn. Seems an uncommonly used function that is prone to trouble as in the Pylint warning. Perhaps we can briefly comment why it is necessary for detached processes?
There was a problem hiding this comment.
Alternatively, we can "upgrade" to using the start_new_session argument described in https://docs.python.org/3.2/library/subprocess.html
There was a problem hiding this comment.
Thanks for the feedback! As this is currently blocking our nightly benchmarks I'll merge this PR as is but this is definitely a good idea for a for a future PR. I'll keep this on my radar! :)
There was a problem hiding this comment.
I've pushed #884.
This change uses the subprocess.run function introduced in Python 3.5 to check the subprocess and print its output to the console on failure. There is some background to why we do not do this currently in #879, but I believe the output is worthwhile, and at the time I think 3.5 was very recently our minimum Python version so this option may have been overlooked... or there may be a more technical reason I am missing.
With this commit we avoid reading from stdout/stderr which might have
been closed by the application and rely solely on the process' return
code.
Relates elastic/elasticsearch#50259