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

Correctly close app body for all code paths #3002

Merged
merged 5 commits into from Nov 3, 2022

Conversation

MSP-Greg
Copy link
Member

@MSP-Greg MSP-Greg commented Oct 26, 2022

Description

  1. At present, some code paths will not close the app body. Add code to always hold a reference to the app body, so when code calling lowlevel_error is executed, the reference to the app body is maintained.

  2. test_rack_server.rb - add test_hijack_body_close - test for above

  3. Small refactor of test_puma_server.rb. Takes too damn long.

  4. test_puma_server.rb - add test_lowlevel_error_body_close to verify body close call when lowlevel_error

Closes #2999.

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.

lib/puma/request.rb Outdated Show resolved Hide resolved
miharekar added a commit to miharekar/visualizer that referenced this pull request Oct 26, 2022
@MSP-Greg MSP-Greg marked this pull request as ready for review October 28, 2022 21:05
@MSP-Greg
Copy link
Member Author

Added commit '[CI] Fix two intermittent failures?', see https://github.com/puma/puma/actions/runs/3348499190

@MSP-Greg
Copy link
Member Author

MSP-Greg commented Nov 2, 2022

One person stated that this PR fixed the issue re #2999, see comment.

Re #3004, as mentioned, testing with hotwired/turbo-rails, I may write a few more tests.

This will cause merge conflicts with #3004, so I'd like to merge this.

Thoughts?

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.0.0 does not close the response bodies of hijacked requests
2 participants