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

Rescue IO::WaitReadable instead of EAGAIN for blocking read #2121

Merged
merged 2 commits into from Feb 21, 2020

Conversation

@wjordan
Copy link
Contributor

wjordan commented Feb 20, 2020

Description

Rescue IO::WaitReadable instead of EAGAIN on calls to read_nonblock when a blocking read would occur.

On Windows, read_nonblock raises Errno::EWOULDBLOCK, which is a different value on this platform from Errno::EAGAIN. Both of these errors are extended by IO::WaitReadable, which is Ruby's recommended exception class for retrying read_nonblock.

I've included a test test_open_connection_wait_no_queue which fails on windows without this PR applied.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.
wjordan added 2 commits Feb 20, 2020
On Windows, `read_nonblock` raises `Errno::EWOULDBLOCK` if a blocking read would occur, which is a different value from `Errno::EAGAIN`. Both of these errors are extended by `IO::WaitReadable` which is Ruby's recommended exception class for retrying `read_nonblock`.
@wjordan wjordan force-pushed the wjordan:io_wait_readable branch from 1bcddae to 3304499 Feb 20, 2020
@wjordan wjordan mentioned this pull request Feb 20, 2020
8 of 8 tasks complete
Copy link
Contributor

olleolleolle left a comment

This is so helpful, and the PR description is eloquent in explaining why this is useful. I hope this can get merged.

@nateberkopec nateberkopec added the bug label Feb 21, 2020
@nateberkopec
Copy link
Member

nateberkopec commented Feb 21, 2020

@MSP-Greg since windows is mentioned, thoughts?

@MSP-Greg
Copy link
Contributor

MSP-Greg commented Feb 21, 2020

LGTM. The set of Errno errors varies by OS, so I'm good. Ruby has several 'adjustments' for things like this in their socket related tests, which run parallel.

Thanks to @wjordan for this. Test suites tend to be written with 'good input' tests. Almost more important are 'bad input' tests, which are needed to make sure the app recovers correctly. Note that 'bad input' can be accidental or deliberate...

@nateberkopec nateberkopec merged commit adb6170 into puma:master Feb 21, 2020
20 checks passed
20 checks passed
build
Details
ubuntu-18.04, 2.3
Details
ubuntu-18.04, 2.4
Details
ubuntu-18.04, 2.5
Details
ubuntu-18.04, 2.6
Details
ubuntu-18.04, 2.7
Details
ubuntu-18.04, ruby-head
Details
macos, 2.3
Details
macos, 2.4
Details
macos, 2.5
Details
macos, 2.6
Details
macos, 2.7
Details
macos, ruby-head
Details
windows-latest, 2.3
Details
windows-latest, 2.4
Details
windows-latest, 2.5
Details
windows-latest, 2.6
Details
windows-latest, 2.7
Details
windows-latest, ruby-head
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.