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

Don't spin on empty fds in read2 on Unix #5030

Merged
merged 1 commit into from
Feb 12, 2018

Conversation

alexcrichton
Copy link
Member

This commit fixes what I think is some pathological behavior in Cargo where if
one stdio stream is closed before another then Cargo can accidentally spin in a
tight loop and not block appropriately. Previously, for example, if stderr
closed before stdout then Cargo would spin in a poll loop continuously getting
notified that stderr is closed.

The behavior is now changed so after a file descriptor is done we stop passing
it to poll and instead only pass the one remaining readable file descriptor.

This commit fixes what I think is some pathological behavior in Cargo where if
one stdio stream is closed before another then Cargo can accidentally spin in a
tight loop and not block appropriately. Previously, for example, if stderr
closed before stdout then Cargo would spin in a `poll` loop continuously getting
notified that stderr is closed.

The behavior is now changed so after a file descriptor is done we stop passing
it to `poll` and instead only pass the one remaining readable file descriptor.
@rust-highfive
Copy link

r? @matklad

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

@matklad
Copy link
Member

matklad commented Feb 12, 2018

@bors r+

The widows case presumable doesn't need similar handling?

@bors
Copy link
Contributor

bors commented Feb 12, 2018

📌 Commit ec991eb has been approved by matklad

@bors
Copy link
Contributor

bors commented Feb 12, 2018

⌛ Testing commit ec991eb with merge a99c7c0619e922c29da9148b15143e5640129b93...

@bors
Copy link
Contributor

bors commented Feb 12, 2018

💡 This pull request was already approved, no need to approve it again.

  • This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.

@bors
Copy link
Contributor

bors commented Feb 12, 2018

📌 Commit ec991eb has been approved by matklad

@bors
Copy link
Contributor

bors commented Feb 12, 2018

⌛ Testing commit ec991eb with merge cb30fba...

bors added a commit that referenced this pull request Feb 12, 2018
Don't spin on empty fds in `read2` on Unix

This commit fixes what I think is some pathological behavior in Cargo where if
one stdio stream is closed before another then Cargo can accidentally spin in a
tight loop and not block appropriately. Previously, for example, if stderr
closed before stdout then Cargo would spin in a `poll` loop continuously getting
notified that stderr is closed.

The behavior is now changed so after a file descriptor is done we stop passing
it to `poll` and instead only pass the one remaining readable file descriptor.
@alexcrichton
Copy link
Member Author

@matklad nah the Windows case I believe already handles this properly (IOCP is way different from nonblocking I/O)

@Mark-Simulacrum
Copy link
Member

Could you also make a PR against compiletest which has this same file copied into it?

@bors
Copy link
Contributor

bors commented Feb 12, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: matklad
Pushing cb30fba to master...

@bors bors merged commit ec991eb into rust-lang:master Feb 12, 2018
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Feb 12, 2018
This was originally copied over from Cargo and Cargo has since [been
updated][update] so let's pull in the fixes here too!

[update]: rust-lang/cargo#5030
kennytm added a commit to kennytm/rust that referenced this pull request Feb 13, 2018
…imulacrum

Update compiletest's `read2` function

This was originally copied over from Cargo and Cargo has since [been
updated][update] so let's pull in the fixes here too!

[update]: rust-lang/cargo#5030
kennytm added a commit to kennytm/rust that referenced this pull request Feb 14, 2018
…imulacrum

Update compiletest's `read2` function

This was originally copied over from Cargo and Cargo has since [been
updated][update] so let's pull in the fixes here too!

[update]: rust-lang/cargo#5030
@alexcrichton alexcrichton deleted the better-poll branch March 1, 2018 22:47
djrenren pushed a commit to djrenren/compiletest that referenced this pull request Aug 26, 2019
This was originally copied over from Cargo and Cargo has since [been
updated][update] so let's pull in the fixes here too!

[update]: rust-lang/cargo#5030
@ehuss ehuss added this to the 1.25.0 milestone Feb 6, 2022
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.

6 participants