Skip to content

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

Merged
bors merged 1 commit into
rust-lang:masterfrom
alexcrichton:better-poll
Feb 12, 2018
Merged

bors merged 1 commit into
rust-lang:masterfrom
alexcrichton:better-poll

Conversation

@alexcrichton

Copy link
Copy Markdown
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
Copy Markdown

r? @matklad

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

@matklad

matklad commented Feb 12, 2018 •

Copy link
Copy Markdown
Contributor

@bors r+

The widows case presumable doesn't need similar handling?

@bors

bors commented Feb 12, 2018

Copy link
Copy Markdown
Contributor

📌 Commit ec991eb has been approved by matklad

@bors

bors commented Feb 12, 2018

Copy link
Copy Markdown
Contributor

⌛ Testing commit ec991eb with merge a99c7c0619e922c29da9148b15143e5640129b93...

@bors

bors commented Feb 12, 2018

Copy link
Copy Markdown
Contributor

💡 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

bors commented Feb 12, 2018

Copy link
Copy Markdown
Contributor

📌 Commit ec991eb has been approved by matklad

@bors

bors commented Feb 12, 2018

Copy link
Copy Markdown
Contributor

⌛ 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
Copy Markdown
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
Copy Markdown
Member

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

@bors

bors commented Feb 12, 2018

Copy link
Copy Markdown
Contributor

☀️ 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