Skip to content

Add response_finished arity to Rack::Lint#2365

Merged
ioquatix merged 2 commits intorack:mainfrom
skipkayhil:hm-spksmupuspyxpolz
Aug 8, 2025
Merged

Add response_finished arity to Rack::Lint#2365
ioquatix merged 2 commits intorack:mainfrom
skipkayhil:hm-spksmupuspyxpolz

Conversation

@skipkayhil
Copy link
Copy Markdown
Contributor

@skipkayhil skipkayhil commented Aug 6, 2025

Ref rails/rails#55425
Ref rails/rails#55458

If these callbacks must be called with four arguments, then this will help ensure that happens.

jeremyevans
jeremyevans reviewed Aug 6, 2025
View reviewed changes
Comment thread lib/rack/lint.rb Outdated
@skipkayhil
Copy link
Copy Markdown
Contributor Author

skipkayhil commented Aug 6, 2025

Note:

Pitchfork currently just .calls when the callable.arity == 0 (because that's how after_reply was implemented), so this would disallow that

https://github.com/Shopify/pitchfork/blob/18869d2f02549a54d7b2db6e0351e7fa71e95546/lib/pitchfork/http_server.rb#L897-L899

The open PR to puma would not need any changes with this extra validation:

puma/puma#3681

@jeremyevans
Copy link
Copy Markdown
Contributor

jeremyevans commented Aug 6, 2025

Pitchfork currently just .calls when the callable.arity == 0 (because that's how after_reply was implemented), so this would disallow that

https://github.com/Shopify/pitchfork/blob/18869d2f02549a54d7b2db6e0351e7fa71e95546/lib/pitchfork/http_server.rb#L897-L899

The open PR to puma would not need any changes with this extra validation:

puma/puma#3681

I think it's a good time for this type of change. We just released Rack 3.2, so Rack 3.3 is not coming any time soon, and servers will have plenty of time to adjust before we release Rack 3.3.

@skipkayhil skipkayhil force-pushed the hm-spksmupuspyxpolz branch from cb01a88 to db61af4 Compare August 7, 2025 20:57
skipkayhil
skipkayhil commented Aug 7, 2025
View reviewed changes
Comment thread lib/rack/lint.rb
@ioquatix ioquatix requested a review from jeremyevans August 7, 2025 22:59
@ioquatix
Copy link
Copy Markdown
Member

ioquatix commented Aug 7, 2025 

Error:
ExecutorTest#test_complete_callbacks_are_called_on_rack_response_finished:
Rack::Lint::LintError: One of SCRIPT_NAME or PATH_INFO must be set (make PATH_INFO '/' if SCRIPT_NAME is empty)
    /home/runner/work/rack/rack/lib/rack/lint.rb:200:in 'Rack::Lint::Wrapper#check_environment'
    /home/runner/work/rack/rack/lib/rack/lint.rb:491:in 'Rack::Lint::Wrapper::ResponseFinishedWrapper#call'
    test/dispatch/executor_test.rb:183:in 'ExecutorTest#test_complete_callbacks_are_called_on_rack_response_finished'

bin/rails test /home/runner/work/rack/rack/external/rails/actionpack/test/dispatch/executor_test.rb:173

..E

Error:
ExecutorTest#test_complete_callbacks_are_called_once_on_rack_response_finished_when_exception_is_raised:
Rack::Lint::LintError: One of SCRIPT_NAME or PATH_INFO must be set (make PATH_INFO '/' if SCRIPT_NAME is empty)
    /home/runner/work/rack/rack/lib/rack/lint.rb:200:in 'Rack::Lint::Wrapper#check_environment'
    /home/runner/work/rack/rack/lib/rack/lint.rb:491:in 'Rack::Lint::Wrapper::ResponseFinishedWrapper#call'
    test/dispatch/executor_test.rb:202:in 'ExecutorTest#test_complete_callbacks_are_called_once_on_rack_response_finished_when_exception_is_raised'

@ioquatix
Copy link
Copy Markdown
Member

ioquatix commented Aug 7, 2025

@adrianna-chang-shopify are you able to fix the Rails failures?

@skipkayhil skipkayhil force-pushed the hm-spksmupuspyxpolz branch from db61af4 to 8f34de7 Compare August 8, 2025 00:26
@skipkayhil skipkayhil mentioned this pull request Aug 8, 2025
Merged
@skipkayhil
Copy link
Copy Markdown
Contributor Author

skipkayhil commented Aug 8, 2025

I opened a PR to fix Rails^ and it also found a bug in my last commit, so I updated the implementation/tests a bit more too

jeremyevans
jeremyevans reviewed Aug 8, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, except for one tweak I think we should make.

Comment thread lib/rack/lint.rb Outdated
@skipkayhil skipkayhil force-pushed the hm-spksmupuspyxpolz branch from 8f34de7 to 7410edb Compare August 8, 2025 03:37
@ioquatix
Copy link
Copy Markdown
Member

ioquatix commented Aug 8, 2025

Can you please update the changelog.

@skipkayhil
Add response_finished arity to Rack::Lint
5de3c80 
If these callbacks _must_ be called with four arguments, then this will
help ensure that happens.
@skipkayhil skipkayhil force-pushed the hm-spksmupuspyxpolz branch from 7410edb to 5de3c80 Compare August 8, 2025 14:18
@ioquatix ioquatix merged commit 79d6820 into rack:main Aug 8, 2025
17 checks passed
@skipkayhil skipkayhil deleted the hm-spksmupuspyxpolz branch August 9, 2025 02:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeremyevans jeremyevans jeremyevans left review comments

@ioquatix ioquatix ioquatix approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@skipkayhil @jeremyevans @ioquatix