Add support for rack.response_finished (#3681)#9
Add support for rack.response_finished (#3681)#9MitchLewis930 wants to merge 1 commit intopr_059_beforefrom
rack.response_finished (#3681)#9Conversation
* 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>
📝 WalkthroughWalkthroughAdds support for the Rack Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant "Puma Request\nHandler" as Handler
participant "Rack App" as App
participant "Response\nCallbacks" as Callbacks
Client->>Handler: HTTP request
Handler->>App: call env
App-->>Handler: status, headers, body
Handler->>Client: stream/send response
Note over Handler: Response already sent
Handler->>Callbacks: run env['rack.response_finished'] (reverse)
loop each callback
Callbacks->>Callbacks: invoke callback(env,status,headers,error)
Callbacks-->>Handler: callback errors handled via debug logging
end
Handler->>Handler: finalize request
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/puma/request.rb`:
- Around line 150-158: The callbacks in the RACK_RESPONSE_FINISHED handling can
receive nil because local variables status and headers aren't set on early
returns in handle_request (e.g., closed_socket? and the 413 branch); ensure
status and headers are initialized and updated on those paths so callbacks get
the real response values: initialize status and headers before calling
handle_request (or set them inside the closed_socket? and 413 branches) and
propagate those values into the response_finished.each callback invocation (the
response_finished block that calls o.call(env, status, headers, error)); update
any early-return branches to assign the correct numeric/status and header hash
rather than leaving them nil.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/puma/binder.rblib/puma/const.rblib/puma/request.rbtest/test_rack_server.rb
🧰 Additional context used
🧬 Code graph analysis (2)
test/test_rack_server.rb (1)
lib/puma/log_writer.rb (1)
null(56-59)
lib/puma/request.rb (2)
lib/puma/log_writer.rb (8)
error(98-101)unknown_error(135-137)log(62-68)call(12-146)call(14-18)call(15-17)call(20-24)call(21-23)lib/puma/server.rb (1)
lowlevel_error(564-581)
🔇 Additional comments (8)
lib/puma/binder.rb (1)
47-51: LGTM!The addition of
RACK_AFTER_REPLYandRACK_RESPONSE_FINISHEDtoproto_envis correct. Initializing them tonilhere allows the request handler to lazily initialize them to empty arrays via||= []only when needed.lib/puma/const.rb (1)
231-231: LGTM!The constant follows the established pattern for Rack environment keys and aligns with the Rack specification for
rack.response_finished.lib/puma/request.rb (3)
55-55: LGTM!Good addition of the
errorvariable initialization to capture exceptions for therack.response_finishedcallbacks.
124-132: LGTM!The variable rename from
etoerroris consistent and enables proper error propagation to therack.response_finishedcallbacks.
96-96: LGTM!Lazy initialization to an empty array follows the same pattern as
RACK_AFTER_REPLYon line 95.test/test_rack_server.rb (3)
60-60: LGTM!The server initialization now explicitly passes a null log writer, which is appropriate for test isolation.
199-223: LGTM!Good test coverage for the
rack.response_finishedcallback mechanism:
- Verifies callbacks receive correct parameters (
env,status,headers,error)- Confirms LIFO execution order (
[3, 2, 1])- Validates that exceptions in callbacks don't prevent subsequent callbacks from executing
225-249: LGTM!Good coverage for the error path, verifying that exceptions raised by the app are properly propagated to
rack.response_finishedcallbacks with the correct status (500) and error object.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if response_finished = env[RACK_RESPONSE_FINISHED] | ||
| response_finished.reverse_each do |o| | ||
| begin | ||
| o.call(env, status, headers, error) | ||
| rescue StandardError => e | ||
| @log_writer.debug_error e | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
status and headers may be nil for early return paths.
If handle_request returns early at line 57 (closed_socket?) or line 60 (413 response), the status and headers local variables are never assigned. The callbacks would then receive nil for both parameters.
Notably, for the 413 case, a response is actually sent to the client, but the callback receives nil instead of the actual status (413).
Consider initializing status and headers before potential early returns, or updating them in those paths:
🛠️ Possible approach
def handle_request(client, requests)
env = client.env
io_buffer = client.io_buffer
socket = client.io # io may be a MiniSSL::Socket
app_body = nil
error = nil
+ status = nil
+ headers = nil
return false if closed_socket?(socket)
if client.http_content_length_limit_exceeded
+ status = 413
+ headers = {}
return prepare_response(413, {}, ["Payload Too Large"], requests, client)
end🤖 Prompt for AI Agents
In `@lib/puma/request.rb` around lines 150 - 158, The callbacks in the
RACK_RESPONSE_FINISHED handling can receive nil because local variables status
and headers aren't set on early returns in handle_request (e.g., closed_socket?
and the 413 branch); ensure status and headers are initialized and updated on
those paths so callbacks get the real response values: initialize status and
headers before calling handle_request (or set them inside the closed_socket? and
413 branches) and propagate those values into the response_finished.each
callback invocation (the response_finished block that calls o.call(env, status,
headers, error)); update any early-return branches to assign the correct
numeric/status and header hash rather than leaving them nil.
2 participants
PR_059
Summary by CodeRabbit
New Features
Tests