Add support for rack.response_finished
#3681
Conversation
|
Thanks for these PR's. Question, which I've wondered about, but never tested: Given # assume env["rack.response_finished"] is allowed to be nil
if temp = env["rack.response_finished"]
temp.each { # some op }
end-- or -- temp = env["rack.response_finished"] || []
temp.each { # some op }Obviously, we probably shouldn't create extra arrays, or create one for every request, so the code above wouldn't be exactly the same if added to Puma... |
The first version saves an allocation, and saves a method call if the value is But since the key is pretty much guaranteed to be there (unless the application delete the key for some reason), it really doesn't make any substantial difference in reality. |
|
Sorry. Posted before I consumed enough coffee. Kind of rooted in a frustration for a method similar to I may do a future PR adding these keys to Binder's Thanks. |
|
I merged in your other PR this one now has a conflict. I'm trying to get the keepalive fix out the door, to do that I'm trying to cut a release 6.6.1 and then have main target v7. As this is an addition, it would need to either be 6.7 (would need a new codename) or go out in 7.x. |
Right, I wasn't sure if all keys needed a constant or not. I can easily add it. |
b283739 to
ba88f0a
Compare
|
Rebased. |
| @@ -92,6 +92,7 @@ def handle_request(client, requests) | |||
| # array, we will invoke them when the request is done. | |||
| # | |||
| env[RACK_AFTER_REPLY] ||= [] | |||
| env[RACK_RESPONSE_FINISHED] ||= [] | |||
There was a problem hiding this comment.
| env[RACK_RESPONSE_FINISHED] ||= [] | |
| response_finished = [] | |
| env[RACK_RESPONSE_FINISHED] = response_finished |
Do we actually need ||= here?
lib/puma/request.rb
Outdated
| if response_finished = env[RACK_RESPONSE_FINISHED] | ||
| response_finished.reverse_each do |o| | ||
| begin | ||
| o.call | ||
| rescue StandardError => e | ||
| @log_writer.debug_error e | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
| if response_finished = env[RACK_RESPONSE_FINISHED] | |
| response_finished.reverse_each do |o| | |
| begin | |
| o.call | |
| rescue StandardError => e | |
| @log_writer.debug_error e | |
| end | |
| end | |
| end | |
| response_finished.reverse_each do |o| | |
| begin | |
| o.call | |
| rescue StandardError => e | |
| @log_writer.debug_error e | |
| end | |
| end |
There was a problem hiding this comment.
Thanks.
Re the actual call, should it be something similar to:
o.call(env, status, headers, e)There was a problem hiding this comment.
Yes, that's correct. This is now strictly clarified in the rack-head spec, so if you run against Rack::Lint in Rack::Head and have some tests for this, it will validate the callback.
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>
ba88f0a to
a745614
Compare
|
I updated the PR to pass |
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: #3631 (The PR seems stalled?)
Ref: rack/rack#1777
Ref: rack/rack#1802