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

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`.
Copy link
Contributor

@olleolleolle olleolleolle left a comment

Choose a reason for hiding this comment

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

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

@nateberkopec
Copy link
Member

@MSP-Greg since windows is mentioned, thoughts?

@nateberkopec nateberkopec added the waiting-for-review Waiting on review from anyone label Feb 21, 2020
@MSP-Greg
Copy link
Member

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 removed the waiting-for-review Waiting on review from anyone label Feb 21, 2020
@nateberkopec nateberkopec merged commit adb6170 into puma:master Feb 21, 2020
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Sep 8, 2020
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Sep 8, 2020
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Sep 8, 2020
Co-authored-by: wjordan <will@code.org>
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Sep 8, 2020
Co-authored-by: wjordan <will@code.org>
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Sep 8, 2020
Co-authored-by: wjordan <will@code.org>
nateberkopec pushed a commit that referenced this pull request Sep 24, 2020
* Backport ssl fixtures/changes from #2333

* RuboCop server.rb

* Update Actions workflow, add Ubuntu 20.04

* Update extconf.rb

* Backport #2121

Co-authored-by: wjordan <will@code.org>

Co-authored-by: wjordan <will@code.org>
MSP-Greg added a commit to MSP-Greg/puma that referenced this pull request Nov 28, 2020
…#2380)

* Backport ssl fixtures/changes from puma#2333

* RuboCop server.rb

* Update Actions workflow, add Ubuntu 20.04

* Update extconf.rb

* Backport puma#2121

Co-authored-by: wjordan <will@code.org>

Co-authored-by: wjordan <will@code.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants