Add suport for rack.response_finished #3631

robertlaurin · 2025-03-13T21:38:46Z

Description

Looking to add support for racks response finished hooks.

rack.response_finished

An array of callables run by the server after the response has been processed. This would typically be invoked after sending the response to the client, but it could also be invoked if an error occurs while generating the response or sending the response; in that case, the error argument will be a subclass of Exception. The callables are invoked with +env, status, headers, error+ arguments and should not raise any exceptions. They should be invoked in reverse order of registration.

It is my understanding that the expected use of rack.after_reply and rack.response_finished are the same, so for this PR I opted to have them share the callable array instead of duplicating the after_reply behaviour just for response_finished.

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.

ahx · 2025-05-12T09:20:41Z

This is great. Being able to use the default "rack.response_finished" or Rack::RACK_RESPONSE_FINISHED as specified by rack would be super helpful, because we could make use of Rack::Lint and would help making people aware of the existence of this feature and would make people feel more confident about using it.

Copilot

Pull Request Overview

This PR adds support for the rack.response_finished hook by sharing its callable array with rack.after_reply.

New tests in test/test_rack_server.rb to verify both normal and exceptional behavior of rack.response_finished.
Updates to lib/puma/request.rb to assign rack.response_finished the same array as rack.after_reply.
Addition of the RACK_RESPONSE_FINISHED constant in lib/puma/const.rb.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
test/test_rack_server.rb	Added tests to verify normal and exception behaviors for rack.response_finished.
lib/puma/request.rb	Assigned rack.response_finished to share the callable array with rack.after_reply.
lib/puma/const.rb	Added a new constant RACK_RESPONSE_FINISHED to support the new functionality.

Copilot · 2025-05-12T12:04:41Z

test/test_rack_server.rb

+    sleep 0.1
+    assert_raises IO::WaitReadable do
+      content = socket.read_nonblock(12)
+      refute_includes content, "500"


[nitpick] Using a fixed sleep duration can potentially lead to flaky tests if the timing varies. Consider replacing it with a polling or waiting mechanism to check for the expected behavior.

Suggested change

sleep 0.1

assert_raises IO::WaitReadable do

content = socket.read_nonblock(12)

refute_includes content, "500"

# Wait for the socket to become readable within 0.1 seconds.

readable, = IO.select([socket], nil, nil, 0.1)

if readable

content = socket.read_nonblock(12)

refute_includes content, "500"

else

# If the socket is not readable, assume no extra content was written.

assert_raises IO::WaitReadable do

socket.read_nonblock(12)

end

Suggested change may not be exactly what we should do but I agree with the intent of the suggestion

Yeah to be honest I just mirrored the test for rack.after_reply, if the maintainers want to see this test updated I can also make those changes there as well.

lib/puma/request.rb

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

kirs · 2025-06-19T22:14:47Z

@nateberkopec 👋 would appreciate your 👀 on this. Thanks!

MSP-Greg · 2025-06-20T00:36:10Z

This looks like a good addition.

Looking at rack/rack#1777, rack/rack#1802, rack/rack#1952, and Shopify/pitchfork#98.

Not sure if the call loop should be updated to match what the pitchfork code did, I think one or more of the Puma call loops was/were changed to rescue within the loop rather than outside of it. That was part of the pitchfork PR.

robertlaurin · 2025-06-20T14:29:05Z

Not sure if the call loop should be updated to match what the pitchfork code did

Adding more reliability to these hooks is a desirable trait in my opinion, should I make that change as part of this PR?

adrianna-chang-shopify · 2025-07-16T17:43:28Z

Just making note that, based on the SPEC, it appears that the callables should be executed in reverse order. I just opened a PR in pitchfork to fix this.

MSP-Greg · 2025-07-28T14:29:27Z

rack.response_finished should be added to Puma.

Add rack.after_reply to simplify logging and resources was added 20-Oct-2011. It also exists in Unicorn.

Since the rack spec does state 'They should be invoked in reverse order of registration', I think it would be best to reverse the call array for rack.response_finished, and leave rack.after_reply's behavior as is.

Given the current behavior, maybe Puma should run the rack.after_reply calls first, and then run the rack.response_finished calls in reverse order?

Lastly, as mentioned previously, should these calls be rescued inside or outside of the loop?

It is very similar to `rack.after_reply`, but is part of the Rack spec. It can't just be an alias because the spec state it has to invoke callbacks in reverse order. Fix: puma#3631 Ref: rack/rack#1777 Ref: rack/rack#1802

It is very similar to `rack.after_reply`, but is part of the Rack spec. It can't just be an alias because the spec state it has to invoke callbacks in reverse order. Fix: puma#3631 Ref: rack/rack#1777 Ref: rack/rack#1802 Co-Authored-By: Robert Laurin <robert.laurin@shopify.com>

* Add support for `rack.response_finished` It is very similar to `rack.after_reply`, but is part of the Rack spec. It can't just be an alias because the spec state it has to invoke callbacks in reverse order. Fix: puma#3631 Ref: rack/rack#1777 Ref: rack/rack#1802 Co-Authored-By: Robert Laurin <robert.laurin@shopify.com> * request.rb - more rename `e` to `error` changes --------- Co-authored-by: Robert Laurin <robert.laurin@shopify.com> Co-authored-by: MSP-Greg <Greg.mpls@gmail.com>

github-actions bot added the waiting-for-review Waiting on review from anyone label Mar 13, 2025

Add suport for rack.response_finished

6628ab9

robertlaurin force-pushed the rack-response-finished branch from 9c9cf7c to 6628ab9 Compare April 8, 2025 17:48

dentarg added the feature label May 12, 2025

dentarg requested a review from Copilot May 12, 2025 12:04

Copilot AI reviewed May 12, 2025

View reviewed changes

robertlaurin and others added 3 commits June 5, 2025 12:51

Merge branch 'master' into rack-response-finished

6d2321a

Merge branch 'master' into rack-response-finished

2944653

Add clarification comment

4c98c6e

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

byroot mentioned this pull request Jul 31, 2025

Add support for rack.response_finished #3681

Merged

MSP-Greg closed this in #3681 Aug 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add suport for rack.response_finished #3631

Add suport for rack.response_finished #3631

Uh oh!

robertlaurin commented Mar 13, 2025 •

edited

Loading

Uh oh!

ahx commented May 12, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI May 12, 2025

Uh oh!

nvasilevski Jun 19, 2025

Uh oh!

robertlaurin Jun 20, 2025

Uh oh!

Uh oh!

kirs commented Jun 19, 2025

Uh oh!

MSP-Greg commented Jun 20, 2025

Uh oh!

robertlaurin commented Jun 20, 2025

Uh oh!

adrianna-chang-shopify commented Jul 16, 2025

Uh oh!

MSP-Greg commented Jul 28, 2025

Uh oh!

Uh oh!

Add suport for rack.response_finished #3631

Add suport for rack.response_finished #3631

Uh oh!

Conversation

robertlaurin commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

rack.response_finished

Your checklist for this pull request

Uh oh!

ahx commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 12, 2025

Choose a reason for hiding this comment

Uh oh!

nvasilevski Jun 19, 2025

Choose a reason for hiding this comment

Uh oh!

robertlaurin Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kirs commented Jun 19, 2025

Uh oh!

MSP-Greg commented Jun 20, 2025

Uh oh!

robertlaurin commented Jun 20, 2025

Uh oh!

adrianna-chang-shopify commented Jul 16, 2025

Uh oh!

MSP-Greg commented Jul 28, 2025

Uh oh!

Uh oh!

robertlaurin commented Mar 13, 2025 •

edited

Loading

ahx commented May 12, 2025 •

edited

Loading