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

Always send lowlevel_error response to client #2731

Merged
merged 8 commits into from Nov 2, 2021

Conversation

baelter
Copy link
Contributor

@baelter baelter commented Oct 27, 2021

Description

Always send what the lowlevel_error handler returns to the client.

Refactors some code out of Request#handle_request to a new Request#write_response that can be used from Server to send the response from lowlevel_error
Closes #2341

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) 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.

@baelter baelter marked this pull request as ready for review Oct 28, 2021
test/test_puma_server.rb Outdated Show resolved Hide resolved
@dentarg dentarg requested a review from cjlarose Oct 28, 2021
@cjlarose cjlarose self-assigned this Oct 28, 2021
@dentarg dentarg force-pushed the always-return-lowlevel-errors branch from 228d4a3 to 388141a Compare Oct 29, 2021
@dentarg dentarg force-pushed the always-return-lowlevel-errors branch from 388141a to 40c72f9 Compare Oct 31, 2021
@dentarg dentarg added the waiting-for-review label Oct 31, 2021
@nateberkopec
Copy link
Member

@nateberkopec nateberkopec commented Nov 1, 2021

Thank you for trying to close such a long-standing issue!

test/test_puma_server.rb Outdated Show resolved Hide resolved
@cjlarose cjlarose removed their assignment Nov 2, 2021
Copy link
Member

@cjlarose cjlarose left a comment

Awesome work!

@cjlarose
Copy link
Member

@cjlarose cjlarose commented Nov 2, 2021

Changes look good to me. If it looks good to you guys @nateberkopec and @dentarg , I think we're good to merge.

@dentarg
Copy link
Member

@dentarg dentarg commented Nov 2, 2021

I should disclose that @baelter is my co-worker at 84codes, hence I was quick to look at this one. I think a second approve from someone else would be great, maybe @MSP-Greg if @nateberkopec doesn't have the time right now?

@MSP-Greg MSP-Greg merged commit f8acac1 into puma:master Nov 2, 2021
47 checks passed
@baelter
Copy link
Contributor Author

@baelter baelter commented Nov 3, 2021

Awesome work!

Thanks!

dentarg added a commit to dentarg/puma that referenced this issue Jan 26, 2022
nateberkopec pushed a commit that referenced this issue Jan 26, 2022
baelter added a commit to baelter/puma that referenced this issue Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature waiting-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants