Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apt.py: use subprocess.run with check=True instead of check_call when capturing stdout and stderr #114

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

Perfect5th
Copy link
Contributor

It's inadvisable to use check_call with stdout=PIPE and stderr=PIPE, as the child process will block if enough output is generated (see Note in https://docs.python.org/3/library/subprocess.html#subprocess.check_call)

We've seen some hanging subprocesses in the landscape-server charm due to this (and other, already addressed issues): https://bugs.launchpad.net/landscape/+bug/2033091

This change uses subprocess.run instead, with capture_output=True and check=True. Behaviour should be the same, except avoiding the aforementioned blocking issue.

Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM!

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Changes look good to me, however, we need to bump up the LIBPATCH version here:

Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thanks! subprocess.run is nice!

@benhoyt benhoyt merged commit 405948a into canonical:main Dec 14, 2023
5 checks passed
@benhoyt
Copy link
Collaborator

benhoyt commented Dec 14, 2023

Separately, I've noticed that we're using e.output (stdout) in the exception messages, meaning that the actual error message doesn't get included in the output here, it just says "None". See #113. We'll fix that separately.

@benhoyt
Copy link
Collaborator

benhoyt commented Dec 14, 2023

That said, @Perfect5th, if you would like to submit a PR to use e.stderr in those exceptions, go ahead. :-)

(I notice that this change will mean that e.output is b'' instead of None, but that doesn't help much.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants