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.

Sign up for GitHub

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

Jump to bottom

Fix deadlock when build scripts are waiting for input on stdin #11205

Merged
merged 1 commit into from
Oct 12, 2022
Merged

Fix deadlock when build scripts are waiting for input on stdin #11205

merged 1 commit into from
Oct 12, 2022

Conversation

arlosi
Copy link
Contributor

@arlosi arlosi commented Oct 10, 2022 •
edited
Loading

What does this PR try to resolve?

#10738 introduced a regression where build scripts did not close stdin, causing deadlocks for build scripts waiting for stdin.

Fixes #11196 by not piping stdin unless requested by the ProcessBuilder.

How should we test and review this PR?

Run a build script that reads from stdin and check that it no longer hangs. See the POC in the linked issue. The included test hangs without the fix.

r? @ehuss

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 10, 2022
@arlosi
Copy link
Contributor Author

arlosi commented Oct 10, 2022

We may want to beta backport this.

weihanglo
weihanglo reviewed Oct 10, 2022
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

This does blow away the deadlock, but I am not sure whether to keep stdin piped by default. Probably not?

crates/cargo-util/src/process_builder.rs Outdated Show resolved Hide resolved
tests/testsuite/build_script.rs Show resolved Hide resolved
@arlosi
Copy link
Contributor Author

arlosi commented Oct 10, 2022

This does blow away the deadlock, but I am not sure whether to keep stdin piped by default. Probably not?

Okay, I reworked it to avoid opening stdin as piped unless explicitly requested.

weihanglo
weihanglo approved these changes Oct 11, 2022
View reviewed changes
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I wonder if it needs a patch version bump, as it contains no-breaking change only.

@ehuss, do you have any opinion on this patch?

@arlosi
Copy link
Contributor Author

arlosi commented Oct 12, 2022

I wonder if it needs a patch version bump

I bumped the patch version.

@ehuss
Copy link
Contributor

ehuss commented Oct 12, 2022

Thanks!

@bors r+

And yea, if you don't mind, can you post a PR to the rust-1.65.0 branch to backport this?

@bors
Copy link
Collaborator

bors commented Oct 12, 2022

📌 Commit d51ed05 has been approved by ehuss

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 12, 2022
@bors
Copy link
Collaborator

bors commented Oct 12, 2022

⌛ Testing commit d51ed05 with merge 642a0e6...

@bors
Copy link
Collaborator

bors commented Oct 12, 2022

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 642a0e6 to master...

@bors bors merged commit 642a0e6 into rust-lang:master Oct 12, 2022
@bors bors mentioned this pull request Oct 12, 2022
Merged
@weihanglo weihanglo mentioned this pull request Oct 14, 2022
Merged
Dylan-DPC pushed a commit to Dylan-DPC/rust that referenced this pull request Oct 15, 2022
@weihanglo
Update cargo
40bb4b7 
12 commits in b8f30cb23c4e5f20854a4f683325782b7cff9837..b332991a57c9d055f1864de1eed93e2178d49440
2022-10-10 19:16:06 +0000 to 2022-10-13 22:05:28 +0000
- Differentiate the warning when an alias (built-in or user-defined) shadows an external subcommand (rust-lang/cargo#11170)
- chore: Update tests for latest clap (rust-lang/cargo#11235)
- feat(publish): Support 'publish.timeout' config behind '-Zpublish-timeout' (rust-lang/cargo#11230)
- Add missing edition (rust-lang/cargo#11231)
- doc(profiles): add module level doc (rust-lang/cargo#11219)
- refactor(publish): Clarify which SourceId is being used (rust-lang/cargo#11216)
- Add new SourceKind::SparseRegistry to differentiate sparse registries (rust-lang/cargo#11209)
- Fix deadlock when build scripts are waiting for input on stdin (rust-lang/cargo#11205)
- refactor: New variant `FeaturesFor::ArtifactDep` (rust-lang/cargo#11184)
- Fix rustdoc warning about unclosed HTML tag (rust-lang/cargo#11221)
- refactor(tests): Prepare for wait-for-publish test changes (rust-lang/cargo#11210)
- Add configuration option for controlling crates.io protocol (rust-lang/cargo#11215)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 15, 2022
@Dylan-DPC
Rollup merge of rust-lang#103041 - weihanglo:update-cargo, r=ehuss
8437d75 
Update cargo

12 commits in b8f30cb23c4e5f20854a4f683325782b7cff9837..b332991a57c9d055f1864de1eed93e2178d49440 2022-10-10 19:16:06 +0000 to 2022-10-13 22:05:28 +0000
- Differentiate the warning when an alias (built-in or user-defined) shadows an external subcommand (rust-lang/cargo#11170)
- chore: Update tests for latest clap (rust-lang/cargo#11235)
- feat(publish): Support 'publish.timeout' config behind '-Zpublish-timeout' (rust-lang/cargo#11230)
- Add missing edition (rust-lang/cargo#11231)
- doc(profiles): add module level doc (rust-lang/cargo#11219)
- refactor(publish): Clarify which SourceId is being used (rust-lang/cargo#11216)
- Add new SourceKind::SparseRegistry to differentiate sparse registries (rust-lang/cargo#11209)
- Fix deadlock when build scripts are waiting for input on stdin (rust-lang/cargo#11205)
- refactor: New variant `FeaturesFor::ArtifactDep` (rust-lang/cargo#11184)
- Fix rustdoc warning about unclosed HTML tag (rust-lang/cargo#11221)
- refactor(tests): Prepare for wait-for-publish test changes (rust-lang/cargo#11210)
- Add configuration option for controlling crates.io protocol (rust-lang/cargo#11215)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 15, 2022
@Dylan-DPC
Rollup merge of rust-lang#103041 - weihanglo:update-cargo, r=ehuss
27501e2 
Update cargo

12 commits in b8f30cb23c4e5f20854a4f683325782b7cff9837..b332991a57c9d055f1864de1eed93e2178d49440 2022-10-10 19:16:06 +0000 to 2022-10-13 22:05:28 +0000
- Differentiate the warning when an alias (built-in or user-defined) shadows an external subcommand (rust-lang/cargo#11170)
- chore: Update tests for latest clap (rust-lang/cargo#11235)
- feat(publish): Support 'publish.timeout' config behind '-Zpublish-timeout' (rust-lang/cargo#11230)
- Add missing edition (rust-lang/cargo#11231)
- doc(profiles): add module level doc (rust-lang/cargo#11219)
- refactor(publish): Clarify which SourceId is being used (rust-lang/cargo#11216)
- Add new SourceKind::SparseRegistry to differentiate sparse registries (rust-lang/cargo#11209)
- Fix deadlock when build scripts are waiting for input on stdin (rust-lang/cargo#11205)
- refactor: New variant `FeaturesFor::ArtifactDep` (rust-lang/cargo#11184)
- Fix rustdoc warning about unclosed HTML tag (rust-lang/cargo#11221)
- refactor(tests): Prepare for wait-for-publish test changes (rust-lang/cargo#11210)
- Add configuration option for controlling crates.io protocol (rust-lang/cargo#11215)
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Oct 15, 2022
@Dylan-DPC
Rollup merge of rust-lang#103041 - weihanglo:update-cargo, r=ehuss
ac23c9f 
Update cargo

12 commits in b8f30cb23c4e5f20854a4f683325782b7cff9837..b332991a57c9d055f1864de1eed93e2178d49440 2022-10-10 19:16:06 +0000 to 2022-10-13 22:05:28 +0000
- Differentiate the warning when an alias (built-in or user-defined) shadows an external subcommand (rust-lang/cargo#11170)
- chore: Update tests for latest clap (rust-lang/cargo#11235)
- feat(publish): Support 'publish.timeout' config behind '-Zpublish-timeout' (rust-lang/cargo#11230)
- Add missing edition (rust-lang/cargo#11231)
- doc(profiles): add module level doc (rust-lang/cargo#11219)
- refactor(publish): Clarify which SourceId is being used (rust-lang/cargo#11216)
- Add new SourceKind::SparseRegistry to differentiate sparse registries (rust-lang/cargo#11209)
- Fix deadlock when build scripts are waiting for input on stdin (rust-lang/cargo#11205)
- refactor: New variant `FeaturesFor::ArtifactDep` (rust-lang/cargo#11184)
- Fix rustdoc warning about unclosed HTML tag (rust-lang/cargo#11221)
- refactor(tests): Prepare for wait-for-publish test changes (rust-lang/cargo#11210)
- Add configuration option for controlling crates.io protocol (rust-lang/cargo#11215)
@arlosi arlosi mentioned this pull request Oct 19, 2022
Merged
bors added a commit that referenced this pull request Oct 20, 2022
@bors
Auto merge of #11257 - arlosi:build-deadlock-1.65.0, r=weihanglo
4bc8f24 
[BETA-1.65] Fix deadlock when build scripts are waiting for input on stdin

Beta backport of #11205. Cherry picked d51ed05.

r? `@ehuss`
lyming2007 pushed a commit to lyming2007/rust that referenced this pull request Oct 21, 2022
@weihanglo
Update cargo
5985ed4 
12 commits in b8f30cb23c4e5f20854a4f683325782b7cff9837..b332991a57c9d055f1864de1eed93e2178d49440
2022-10-10 19:16:06 +0000 to 2022-10-13 22:05:28 +0000
- Differentiate the warning when an alias (built-in or user-defined) shadows an external subcommand (rust-lang/cargo#11170)
- chore: Update tests for latest clap (rust-lang/cargo#11235)
- feat(publish): Support 'publish.timeout' config behind '-Zpublish-timeout' (rust-lang/cargo#11230)
- Add missing edition (rust-lang/cargo#11231)
- doc(profiles): add module level doc (rust-lang/cargo#11219)
- refactor(publish): Clarify which SourceId is being used (rust-lang/cargo#11216)
- Add new SourceKind::SparseRegistry to differentiate sparse registries (rust-lang/cargo#11209)
- Fix deadlock when build scripts are waiting for input on stdin (rust-lang/cargo#11205)
- refactor: New variant `FeaturesFor::ArtifactDep` (rust-lang/cargo#11184)
- Fix rustdoc warning about unclosed HTML tag (rust-lang/cargo#11221)
- refactor(tests): Prepare for wait-for-publish test changes (rust-lang/cargo#11210)
- Add configuration option for controlling crates.io protocol (rust-lang/cargo#11215)
@ehuss ehuss added this to the 1.66.0 milestone Nov 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weihanglo weihanglo weihanglo approved these changes

Assignees

@ehuss ehuss

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.66.0
Development

Successfully merging this pull request may close these issues.

Build script regression prevents some builds from terminating
5 participants
@arlosi @ehuss @bors @weihanglo @rust-highfive