Skip to content

Comments

Add support for rack.response_finished (#3681)#9

Open
MitchLewis930 wants to merge 1 commit intopr_059_beforefrom
pr_059_after
Open

Add support for rack.response_finished (#3681)#9
MitchLewis930 wants to merge 1 commit intopr_059_beforefrom
pr_059_after

Conversation

@MitchLewis930
Copy link

PR_059

* 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>
@greptile-apps
Copy link

greptile-apps bot commented Jan 30, 2026

Greptile Overview

Greptile Summary

This PR implements support for rack.response_finished callbacks, which allow Rack applications to register callbacks that execute after the response is sent to the client. The callbacks receive the environment, status, headers, and any error that occurred.

Key changes:

  • Added RACK_RESPONSE_FINISHED constant and initialization in the environment
  • Implemented callback execution in the ensure block with reverse iteration (LIFO order)
  • Callbacks receive 4 parameters: env, status, headers, error
  • Error handling ensures one failing callback doesn't prevent others from running
  • Comprehensive test coverage for both success and error scenarios

Critical Issue Found:
The implementation has a bug where status and headers variables are not initialized at the method start, causing NameError when early returns occur (closed socket, hijacked connection, async response). These variables need initialization to nil at the top of handle_request method.

Confidence Score: 2/5

  • Not safe to merge - contains a critical bug that will cause runtime errors
  • The PR introduces a logical error where uninitialized variables (status and headers) are referenced in the ensure block, which will raise NameError exceptions when early returns happen before these variables are assigned
  • lib/puma/request.rb requires immediate attention to fix variable initialization

Important Files Changed

Filename Overview
lib/puma/request.rb Adds rack.response_finished callback support with critical bug: status/headers variables undefined on early returns causing NameError

Sequence Diagram

sequenceDiagram
    participant Client
    participant Server
    participant App as Rack App
    participant Callbacks as rack.response_finished
    
    Client->>Server: HTTP Request
    Server->>Server: Initialize env with rack.response_finished=[]
    Server->>App: call(env)
    App->>Callbacks: Register callback(s)
    App-->>Server: [status, headers, body]
    
    alt Success Path
        Server->>Server: prepare_response(status, headers, body)
        Server->>Client: Send HTTP Response
        Server->>Callbacks: Execute callbacks(env, status, headers, nil)
    else Error Path
        App-->>Server: raise Exception
        Server->>Server: Catch error, set status=500
        Server->>Client: Send Error Response
        Server->>Callbacks: Execute callbacks(env, 500, headers, error)
    else Early Return (Hijack/Async)
        Server->>Server: return :async
        Server->>Callbacks: Execute callbacks(env, nil, nil, nil)
        Note over Callbacks: ⚠️ BUG: status/headers undefined
    end
Loading

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

io_buffer = client.io_buffer
socket = client.io # io may be a MiniSSL::Socket
app_body = nil
error = nil
Copy link

Choose a reason for hiding this comment

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

status and headers should be initialized here to prevent NameError when early returns happen (lines 57, 113, 122) before these variables are assigned

Suggested change
error = nil
error = nil
status = nil
headers = nil
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/puma/request.rb
Line: 55:55

Comment:
`status` and `headers` should be initialized here to prevent `NameError` when early returns happen (lines 57, 113, 122) before these variables are assigned

```suggestion
      error = nil
      status = nil
      headers = nil
```

How can I resolve this? If you propose a fix, please make it concise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants