Skip to content

Conversation

LevitatingBusinessMan
Copy link
Contributor

@LevitatingBusinessMan LevitatingBusinessMan commented Mar 25, 2025

Description

The rack spec now supports bodies responding with nil for to_path.
This is the default behavior of IO objects made with IO.pipe.

This PR has a commit to prevent puma from crashing when to_path returns nil.
It also has a test for pipe response bodies which tests the presence of this bug.

#3634
rack/rack#2318

Closes #3634

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.

@LevitatingBusinessMan LevitatingBusinessMan changed the title To path accept to_path to be nil on bodies Mar 25, 2025
@github-actions github-actions bot added the waiting-for-review Waiting on review from anyone label Mar 25, 2025
@dentarg
Copy link
Member

dentarg commented Mar 25, 2025

(CI failures seems unrelated, I have filled socketry/localhost#35)

@MSP-Greg
Copy link
Member

@LevitatingBusinessMan

Could you rebase this? CI is passing now.

Also, there are some failures, most due to the issue of 'character' handling vs 'byte' handling.

I added two commits to https://github.com/MSP-Greg/puma/commits/00-pr-3535/ which contain changes to the tests and also changes to puma_socket.rb. All the tests pass with the two additional commits.

Old Windows Rubies (2.4 thru 2.6) seem to be failing, so I skipped them. All Windows Rubies from 2.7 and later are passing.

@LevitatingBusinessMan LevitatingBusinessMan force-pushed the to_path branch 2 times, most recently from a9fccf6 to d43ae7e Compare March 31, 2025 02:05
@LevitatingBusinessMan
Copy link
Contributor Author

Anything still to be done for this PR?

@LevitatingBusinessMan
Copy link
Contributor Author

Can we continue looking into this? Currently puma does not follow the rack spec and I need to use a workaround for #3634.

@MSP-Greg MSP-Greg added the bug label May 8, 2025
@MSP-Greg MSP-Greg merged commit 979b9b0 into puma:master May 8, 2025
87 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug waiting-for-review Waiting on review from anyone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Streaming body may crash if to_path is nil
3 participants