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 write io_buffer when in "enum bodies" branch. #3113

Merged
merged 3 commits into from Mar 31, 2023

Conversation

collinsauve
Copy link
Contributor

@collinsauve collinsauve commented Mar 30, 2023

Description

Root of the issue is that you can get into the if chunked branch when body.each has nothing in it, so the previously populated io_buffer is never written to the socket.

This used to be the case but was changed on #3072. I'm just moving this line back.

Closes #3112.

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.

@dmke
Copy link

dmke commented Mar 30, 2023

I have applied this patch locally and can confirm that head(404) and render(plain: "", status: 404) now behave as expected.

I don't have access to the codebase where I've noticed this issue, so I can't tell if that broke something else 👀

@collinsauve collinsauve changed the title Fix 3112 - always write io_buffer when in "enum bodies" branch Always write io_buffer when in "enum bodies" branch. Closes #3112 Mar 30, 2023
@MSP-Greg
Copy link
Member

MSP-Greg commented Mar 30, 2023

@collinsauve

Thank you for the PR. I added three tests in a commit at 1aa498d, feel free to use them.

The two array body tests pass, the enum body test fails as shown below:

  1) Failure:
TestPumaServer#test_empty_body_enum [/mnt/c/Greg/GitHub/puma/test/test_puma_server.rb:1567]:
--- expected
+++ actual
@@ -1,6 +1,5 @@
-"HTTP/1.1 404 Not Found\r
-Transfer-Encoding: chunked\r
-\r
-0\r
+# encoding: ASCII-8BIT
+#    valid: true
+"0\r
 \r
"

I also added commit 91ad03d, which has some possible changes for request.rb. When sending requests, Puma doesn't want to buffer a large amount of string data before writing to the client socket, but there is also an interest in minimizing the number of writes. This is especially true with SSL connections.

The code in the commit acts the same as your code when the body is empty, but will combine the header section with the first segment of the body if it isn't empty. There are two other changes that check for nil members of the body.

Thanks again, Greg

EDIT: changed the commits, as Windows Rubies 3.1 and later failed...

@MSP-Greg MSP-Greg added the bug label Mar 30, 2023
@collinsauve
Copy link
Contributor Author

collinsauve commented Mar 31, 2023

Thank you @MSP-Greg! I've cherry-picked those commits onto this branch

@collinsauve collinsauve marked this pull request as ready for review March 31, 2023 02:50
@MSP-Greg
Copy link
Member

@collinsauve

Thank you for all your work.

@MSP-Greg MSP-Greg changed the title Always write io_buffer when in "enum bodies" branch. Closes #3112 Always write io_buffer when in "enum bodies" branch. Mar 31, 2023
@MSP-Greg MSP-Greg merged commit a756c92 into puma:master Mar 31, 2023
63 checks passed
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.

Puma 6.2 does not respond correctly when Rails app responds with empty body
3 participants