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

_subprocess: Fix pip install log window not showing #567

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ All versions prior to 0.0.9 are untracked.
* Fixed a crash on Windows caused by multiple open file handles to
input requirements ([#551](https://github.com/pypa/pip-audit/pull/551))

* Fixed an issue where the log window that we use to display `pip-audit`'s
dependency resolution progress was not showing anything
([#567](https://github.com/pypa/pip-audit/pull/567))
Comment on lines +54 to +56
Copy link
Member

Choose a reason for hiding this comment

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

Needs relocating under [Unreleased] 🙂


## [2.5.0]

### Changed
Expand Down
6 changes: 4 additions & 2 deletions pip_audit/_subprocess.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ def run(args: Sequence[str], *, log_stdout: bool = False, state: AuditState = Au

terminated = False
stdout = b""
stderr = b""

# NOTE: We use `poll()` to control this loop instead of the `read()` call
# to prevent deadlocks. Similarly, `read(size)` will return an empty bytes
Expand All @@ -54,11 +53,14 @@ def run(args: Sequence[str], *, log_stdout: bool = False, state: AuditState = Au
terminated = process.poll() is not None
# NOTE(ww): Buffer size chosen arbitrarily here and below.
stdout += process.stdout.read(4096) # type: ignore
stderr += process.stderr.read(4096) # type: ignore
state.update_state(
f"Running {pretty_args}", stdout.decode(errors="replace") if log_stdout else None
)

# NOTE(alex): For reasons I'm unsure about, reading the stderr stream in
# real time seems to interfere with stdout and cause us to read nothing.
# Let's wait until the process is terminated before reading stderr.
stderr = process.stderr.read() # type: ignore
Comment on lines +60 to +63
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I'm a little worried this might cause deadlocks in some (unlikely) conditions: I can imagine a case where pip writes to stderr before stdout and blocks waiting for the parent's corresponding read, meaning that process.poll() never sees a process termination (and stdout.read(bufsize) always results in an empty read).

Specifically, that should only happen if pip writes enough to stderr to saturate the underlying pipe buffer. But that might happen, so it's probably worth root causing what's going on here instead of deferring the stderr read.

if process.returncode != 0:
raise CalledProcessError(
f"{pretty_args} exited with {process.returncode}",
Expand Down