Use files instead of pipes to capture stdout/stderr #1434
Conversation
| time.sleep(1) | ||
| retry -= 1 | ||
|
|
||
| def read_output(): |
There was a problem hiding this comment.
As discussed, it would be a good idea to use try/catch for this method in the future.
There was a problem hiding this comment.
Thanks, @pgombar. Added error handling and also limited the amount of data we load into memory.
| @@ -78,7 +68,7 @@ def to_s(captured_stdout, stdout_offset, captured_stderr, stderr_offset): | |||
| return to_s(stdout, -1*max_len_each, stderr, -1*max_len_each) | |||
|
|
|||
|
|
|||
| def _destroy_process(process, signal_to_send=signal.SIGKILL): | |||
| def destroy_process(process, signal_to_send=signal.SIGKILL): | |||
There was a problem hiding this comment.
With the capture_from_process_no_timeout and capture_from_process_raw now gone, is this method used anymore? I can see its imported in exthandlers.py but os.killpg() is used in it.
There was a problem hiding this comment.
Thanks for pointing this out, @vrdmr. I reviewed the code and confirmed the functionality on this function is no longer needed so I removed it.
| if env is None: | ||
| env = {} | ||
| env.update(os.environ) | ||
| with tempfile.TemporaryFile(dir=base_dir, mode="w+b") as stdout: |
There was a problem hiding this comment.
Something to discuss - I would recommend redirecting the stdout/stderr in the /var/log/azure/(EXTN)/ directory - If in case the file becomes too large, the user should be able to delete the file forcefully, and need not be root.
There was a problem hiding this comment.
Discussed offline - the log directory is also restricted to root (for writes), and also deleting the file has no effect since the extension still holds a handle to it. When the extension completes, the file will be deleted by the OS.
Addresses #1357
PR information
Quality of Code and Contribution Guidelines
This change is