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

Poll loop fixes #11624

Merged
merged 2 commits into from
Mar 20, 2023
Merged

Poll loop fixes #11624

merged 2 commits into from
Mar 20, 2023

Conversation

vext01
Copy link
Contributor

@vext01 vext01 commented Jan 25, 2023

What does this PR try to resolve?

A couple of minor issues. See individual commits.

How should we test and review this PR?

The existing test suite should have sufficient coverage.

Additional information

(I have 5 apparently unrelated failures locally when running cargo test)

@vext01
Fix incorrect comment.
493ca33 
The event loop uses poll(2), not select(2).
@vext01
Set non-blocking IO more robustly.
5c3825f 
 - Check for errors.
 - Add O_NONBLOCK on top of any existing flags.

set_nonblock() is adapted from lang_tester:
https://github.com/softdevteam/lang_tester/blob/e01072a0a4c5e37f2e585c48efffcf540cd7b6a4/src/tester.rs#L1041-L1048
@rustbot
Copy link
Collaborator

rustbot commented Jan 25, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 25, 2023
@Eh2406
Copy link
Contributor

Eh2406 commented Jan 25, 2023

What does this do/accomplish? How would we know if we break this functionality in the future?

@vext01
Copy link
Contributor Author

vext01 commented Jan 25, 2023 •
edited
Loading

If one of your fcntl() calls fail (and I have seen this happen) then you would be doing blocking IO in the poll loop, likely ending in a deadlock.

@vext01
Copy link
Contributor Author

vext01 commented Jan 25, 2023

Also, getting the current flags and ORing in the non-block flag avoids destructively turning off any other flags the fd may already have set.

@ehuss
Copy link
Contributor

ehuss commented Jan 25, 2023

Can you say what system you are running on and how you have been seeing it fail in cargo?

Although I think this potentially looks more correct, I don't think anybody has reported any issues with the existing code in the past 6 years. So I'd like to better understand why we would be changing this.

ehuss
ehuss reviewed Jan 25, 2023
View reviewed changes
crates/cargo-util/src/read2.rs
use std::io;
use std::io::prelude::*;
use std::mem;
use std::os::unix::prelude::*;
use std::process::{ChildStderr, ChildStdout};

fn set_nonblock(fd: c_int) -> io::Result<()> {
let flags = unsafe { fcntl(fd, F_GETFL) };
Copy link
Contributor

Choose a reason for hiding this comment

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

There is some uncertainty whether or not F_GETFL takes an argument. https://stackoverflow.com/questions/25061656/when-is-the-arg-for-f-getfl-fcntl-command-required has an interesting discussion of the issue. The Open Group specification notes that the arg value is reserved for future growth, without specifying what that means or what it should be set to or whether or not it should be specified. FreeBSD specifically says it is ignored. Very strange.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On OpenBSD and Linux the manual also says the argument is ignored.

@vext01
Copy link
Contributor Author

vext01 commented Jan 26, 2023

Can you say what system you are running on and how you have been seeing it fail in cargo?

To be clear. I've seen fcntl fail in other Rust programs (due to programming errors), not in Cargo.

But regardless, it's good practice to check for errors defensively so that if there is a problem, it happens as close to the cause as possible.

@vext01
Copy link
Contributor Author

vext01 commented Jan 31, 2023

We good to go?

@vext01
Copy link
Contributor Author

vext01 commented Mar 20, 2023

ping?

@ehuss
Copy link
Contributor

ehuss commented Mar 20, 2023

Thanks! I'll go ahead and merge. I'm a little uncomfortable with this change, but other than the uncertainty about F_GETFL's behavior, I don't know of any specific issues. I also generally avoid changing things if they aren't causing any issues.

@bors r+

@bors
Copy link
Contributor

bors commented Mar 20, 2023

📌 Commit 5c3825f 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 Mar 20, 2023
@bors
Copy link
Contributor

bors commented Mar 20, 2023

⌛ Testing commit 5c3825f with merge 16d0495...

@bors
Copy link
Contributor

bors commented Mar 20, 2023

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 16d0495 to master...

@bors bors merged commit 16d0495 into rust-lang:master Mar 20, 2023
@weihanglo weihanglo mentioned this pull request Mar 21, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 22, 2023
@bors
Auto merge of rust-lang#109463 - weihanglo:update-cargo, r=weihanglo
6dc3999 
Update cargo

11 commits in 4a3c588b1f0a8e2dc8dd8789dbf3b6a71b02ed49..15d090969743630bff549a1b068bcaa8174e5ee3
2023-03-14 14:05:36 +0000 to 2023-03-21 17:54:28 +0000
- docs(contrib): Move higher level resolver docs into doc comments (rust-lang/cargo#11870)
- docs(contrib): Pull impl info out of architecture (rust-lang/cargo#11869)
- Update curl-sys (rust-lang/cargo#11871)
- Poll loop fixes (rust-lang/cargo#11624)
- clippy: warn `disallowed_methods` for `std::env::var` and friends (rust-lang/cargo#11828)
- Add --ignore-rust-version flag to cargo install (rust-lang/cargo#11859)
- Handle case mismatches when looking up env vars in the Config snapshot (rust-lang/cargo#11824)
- align semantics of generated vcs ignore files (rust-lang/cargo#11855)
- Add more information to wait-for-publish (rust-lang/cargo#11713)
- docs: Address warnings (rust-lang/cargo#11856)
- docs(contrib): Create a file overview in the nightly docs (rust-lang/cargo#11850)
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Mar 22, 2023
@bors
Auto merge of #109463 - weihanglo:update-cargo, r=weihanglo
0ed98a9 
Update cargo

11 commits in 4a3c588b1f0a8e2dc8dd8789dbf3b6a71b02ed49..15d090969743630bff549a1b068bcaa8174e5ee3
2023-03-14 14:05:36 +0000 to 2023-03-21 17:54:28 +0000
- docs(contrib): Move higher level resolver docs into doc comments (rust-lang/cargo#11870)
- docs(contrib): Pull impl info out of architecture (rust-lang/cargo#11869)
- Update curl-sys (rust-lang/cargo#11871)
- Poll loop fixes (rust-lang/cargo#11624)
- clippy: warn `disallowed_methods` for `std::env::var` and friends (rust-lang/cargo#11828)
- Add --ignore-rust-version flag to cargo install (rust-lang/cargo#11859)
- Handle case mismatches when looking up env vars in the Config snapshot (rust-lang/cargo#11824)
- align semantics of generated vcs ignore files (rust-lang/cargo#11855)
- Add more information to wait-for-publish (rust-lang/cargo#11713)
- docs: Address warnings (rust-lang/cargo#11856)
- docs(contrib): Create a file overview in the nightly docs (rust-lang/cargo#11850)
@ehuss ehuss added this to the 1.70.0 milestone Mar 26, 2023
@ehuss ehuss mentioned this pull request Apr 23, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehuss ehuss ehuss left review comments

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.70.0
Development

Successfully merging this pull request may close these issues.
5 participants
@vext01 @rustbot @Eh2406 @ehuss @bors