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

Forward lines from stderr of child process to stdout on the same thread, instead of spawning a thread #940

Merged
merged 1 commit into from
Feb 4, 2024

Conversation

dpaoliello
Copy link
Contributor

cc-rs was redirecting output from stderr of its child processes to a pipe, which was then being read on dedicated thread and written to stdout with a wrapper message.

This pattern caused a few issues:

  • It created a dependency from cc-rs to libc when building the Rust standard library with optimized compiler intrinsics, resulting in the Rust build breaking (see Upgrading to cc@1.0.84 breaks libstd bootstrap #913).
  • With the parallel feature enabled, there was no synchronization between different child processes writing to the pipe, potentially resulting in corrupted output.
  • The thread that was forwarding the lines was wasted, since the main thread was blocking waiting for the child process(es) to exit anyway.

The fix for this is to forward output from stderr to stdout on the current thread. For the parallel feature this required implementing a non-blocking version of BufReader::read_until that consumes whatever data is available and then continues.

Fixes #913

@NobodyXu
Copy link
Collaborator

There're a lot of changes here, it might take me some time to review it.

@dpaoliello
Copy link
Contributor Author

If it helps, I have a build of the Rust toolchain that uses this: https://github.com/rust-lang/rust/actions/runs/7719025147/job/21041489650?pr=119199

src/command_helpers.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
@dpaoliello
Copy link
Contributor Author

@NobodyXu I'm going to have to rework this, since there are two issues:

The first, fixable, issue is that BufReader.fill_buf will only read more if all data in the buffer has been consumed - so if we don't consume everything then fill_buf won't read any more data and so each subsequent call to fill_buf will keep returning the same data.

The second, fundamental, issue is that read will block until data is available or the child exits - so in parallel mode we'll block waiting for the first child's stderr to return something or until that process exits.

There's a few different ways that we can address these:

  1. Accept the limitations of the second issue, and workaround the first issue by replacing the BufReader with our own calls to read into a Vec (as you had suggested for the non-parallel code).
  2. Give up on trying to forward the stderr output while the children are running, and instead forward them all once the child has exited.
  3. Do something like the original code but create one thread per child instead of using a single thread.

Thoughts?

@NobodyXu
Copy link
Collaborator

NobodyXu commented Feb 2, 2024

The second, fundamental, issue is that read will block until data is available or the child exits - so in parallel mode we'll block waiting for the first child's stderr to return something or until that process exits.

If you bring back os_pipe, then you could read without blocking.

@dpaoliello
Copy link
Contributor Author

@NobodyXu ok, I think I have this figured out: for Unix, I'm setting the pipe as non-blocking and checking for WouldBlock. For Windows, I'm using PeekNamedPipe to check for data and returning early if there is none.

I also removed the BufReader and switched to directly reading into the Vec.

src/windows/windows_sys.rs Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
src/parallel/stderr.rs Outdated Show resolved Hide resolved
src/parallel/stderr.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Outdated Show resolved Hide resolved
src/command_helpers.rs Show resolved Hide resolved
src/command_helpers.rs Show resolved Hide resolved
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thanks, it overall LGTM, just two nitpicks I want to discuss with you before I approve and merge it.

src/parallel/stderr.rs Outdated Show resolved Hide resolved
src/parallel/stderr.rs Show resolved Hide resolved
src/command_helpers.rs Show resolved Hide resolved
src/parallel/stderr.rs Show resolved Hide resolved
src/parallel/stderr.rs Outdated Show resolved Hide resolved
@NobodyXu NobodyXu merged commit 227b770 into rust-lang:main Feb 4, 2024
18 checks passed
@NobodyXu
Copy link
Collaborator

NobodyXu commented Feb 4, 2024

Thanj you!

@dpaoliello dpaoliello deleted the nopipe branch February 5, 2024 00:39
@dpaoliello
Copy link
Contributor Author

Can we please get a release with this change?

@NobodyXu
Copy link
Collaborator

NobodyXu commented Feb 5, 2024

Yes, I definitely want to have a release.

There's a regression #902 that needs to be fixed before a release can be done.

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.

Upgrading to cc@1.0.84 breaks libstd bootstrap
2 participants