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

request.rb - fix partial hijack precedence, refactor, add tests #3072

Merged
merged 8 commits into from Feb 17, 2023

Conversation

MSP-Greg
Copy link
Member

@MSP-Greg MSP-Greg commented Feb 3, 2023

Description

This is a refactor of request.rb. I tried to make separate commits for each change, so it may be easier to follow. Commits:

  1. request.rb - set content_length earlier - Request#prepare_response has quite a bit of code that looks at the body returned by the app, and swaps it for another object that is passed to Request#fast_write_response. It also may calculate the content_length. There were several places where resp_info[:content_length] was used. Move content_length = resp_info[:content_length] to before all the code, so the code uses content_length instead of a hash value.

  2. request.rb - only compute content_length if res_body.respond_to? :each - The code mentioned above in Request#prepare_response uses res_body.respond_to?(:each) in several places, but all of it can be guarded with if res_body.respond_to? :each. This commit simply takes the whole outer most conditional, indents it, and adds an if res_body.respond_to? :each conditional.

  3. Move response_hijack code into else clause - given the above, add an else clause and move the partial hijack related code that sets response_hijack into it.

  4. Sprockets::Asset's @source is always a string - turn it into an array with a single value.

  5. Fewer socket write with Enum chunked body - Socket writes take longer than string operations, so use the IOBuffer. Also, I believe some Rails responses are an Enum body with one iteration. This combines the first iteration with the headers.

  6. Add hijack comments - add comments clarifying things, partial, full, etc.

I believe this code doesn't change any behavior, except for one item. The following is current code:

if res_body && !res_body.respond_to?(:each)
  response_hijack = res_body
else
  response_hijack = resp_info[:response_hijack]
end

Note that the resp_body is used if certain conditions are true. The Rack spec states "Servers must ignore the body part of the response tuple when the rack.hijack response header is present". The above code doesn't set the priority to resp_info[:response_hijack]. So, a small bug, fixed in this PR...

Added another commit Rearrange partial hijack tests, add full hijack test - this commit adds a 'full hijack' test, which I couldn't find. It also adds a test TestPumaServerHijack#test_partial_hijack_header_closes_body_correct_precedence. This test fails in master, as it catches the bug mentioned above.

Also, Issue #2999 mentioned an issue with 'full hijack' not closing bodies. The tests added at the time tested 'partial hijack' body closure, not 'full hijack'. Updated tests so both full and partial hijack body closure is tested.

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.

@MSP-Greg
Copy link
Member Author

MSP-Greg commented Feb 3, 2023

GitHub seems to be having some network issues, the failed Actions jobs that I looked at all failed in setup-ruby. I'll re-run them when GitHub fixes things, which will be when?

EDIT: Re-ran the failing tests once, all passed.

@MSP-Greg MSP-Greg force-pushed the 00-request-refactor branch 3 times, most recently from 3e84237 to 169d1e9 Compare February 4, 2023 21:42
lib/puma/request.rb Outdated Show resolved Hide resolved
test/test_puma_server_hijack.rb Outdated Show resolved Hide resolved
@MSP-Greg
Copy link
Member Author

Rebased.

@MSP-Greg MSP-Greg changed the title request.rb refactor request.rb - fix partial hijack precedence, refactor, add tests Feb 12, 2023
@MSP-Greg MSP-Greg added the bug label Feb 12, 2023
@MSP-Greg MSP-Greg merged commit 4d8b7c0 into puma:master Feb 17, 2023
@MSP-Greg MSP-Greg deleted the 00-request-refactor branch February 17, 2023 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants