Improved handling of rack.response_finished.

Author: samuel-williams-shopify

lib/protocol/rack/adapter/generic.rb

@@ -135,6 +135,38 @@ def make_environment(request)
 					}
 				end
 
+				# Handle errors that occur during request processing. Logs the error, closes any response body, invokes `rack.response_finished` callbacks, and returns an appropriate failure response.
+				# 
+				# The `rack.response_finished` callbacks are invoked in reverse order of registration, as specified by the Rack specification. If a callback raises an exception, it is caught and logged, but does not prevent other callbacks from being invoked.
+				# 
+				# @parameter env [Hash] The Rack environment hash.
+				# @parameter status [Integer | Nil] The HTTP status code, if available. May be `nil` if the error occurred before the application returned a response.
+				# @parameter headers [Hash | Nil] The response headers, if available. May be `nil` if the error occurred before the application returned a response.
+				# @parameter body [Object | Nil] The response body, if available. May be `nil` if the error occurred before the application returned a response.
+				# @parameter error [Exception] The exception that occurred during request processing.
+				# @returns [Protocol::HTTP::Response] A failure response representing the error.
+				def handle_error(env, status, headers, body, error)
+					Console.error(self, "Error occurred during request processing:", error)
+					
+					# Close the response body if it exists and supports closing.
+					body&.close if body.respond_to?(:close)
+					
+					# Invoke `rack.response_finished` callbacks in reverse order of registration.
+					# This ensures that callbacks registered later are invoked first, matching the Rack specification.
+					env&.[](RACK_RESPONSE_FINISHED)&.reverse_each do |callback|
+						begin
+							callback.call(env, status, headers, error)
+						rescue => callback_error
+							# If a callback raises an exception, log it but continue invoking other callbacks.
+							# The Rack specification states that callbacks should not raise exceptions, but we handle
+							# this gracefully to prevent one misbehaving callback from breaking others.
+							Console.error(self, "Error occurred during response finished callback:", callback_error)
+						end
+					end
+					
+					return failure_response(error)
+				end
+				
 				# Build a rack `env` from the incoming request and apply it to the rack middleware.
 				#
 				# @parameter request [Protocol::HTTP::Request] The incoming request.
@@ -158,16 +190,8 @@ def call(request)
 					headers, meta = self.wrap_headers(headers)
 
 					return Response.wrap(env, status, headers, meta, body, request)
-				rescue => exception
-					Console.error(self, exception)
-					
-					body&.close if body.respond_to?(:close)
-					
-					env&.[](RACK_RESPONSE_FINISHED)&.each do |callback|
-						callback.call(env, status, headers, exception)
-					end
-					
-					return failure_response(exception)
+				rescue => error
+					return self.handle_error(env, status, headers, body, error)
 				end
 
 				# Generate a suitable response for the given exception.

lib/protocol/rack/adapter/rack2.rb

@@ -111,17 +111,8 @@ def call(request)
 					# end
 
 					return Response.wrap(env, status, headers, meta, body, request)
-				rescue => exception
-					Console.error(self, exception)
-					
-					body&.close if body.respond_to?(:close)
-					
-					# Rack 2 does not include `rack.response_finished` in the specification. However, if the application has set it, we will call the callbacks here as it would be extremely surprising to not do so.
-					env&.[](RACK_RESPONSE_FINISHED)&.each do |callback|
-						callback.call(env, status, headers, exception)
-					end
-					
-					return failure_response(exception)
+				rescue => error
+					return self.handle_error(env, status, headers, body, error)
 				end
 
 				# Process the rack response headers into a {Protocol::HTTP::Headers} instance, along with any extra `rack.` metadata.

lib/protocol/rack/body.rb

@@ -7,6 +7,7 @@
 require_relative "body/enumerable"
 require_relative "constants"
 
+require "console"
 require "protocol/http/body/completable"
 require "protocol/http/body/head"
 
@@ -103,8 +104,10 @@ def self.wrap(env, status, headers, body, input = nil, head = false)
 				return body
 			end
 
-			# Create a completion callback for response finished handlers.
-			# The callback is called with any error that occurred during response processing.
+			# Create a completion callback for response finished handlers. The callback is called with any error that occurred during response processing.
+			# 
+			# Callbacks are invoked in reverse order of registration, as specified by the Rack specification.
+			# If a callback raises an exception, it is caught and logged, but does not prevent other callbacks from being invoked.
 			# 
 			# @parameter response_finished [Array] Array of response finished callbacks.
 			# @parameter env [Hash] The Rack environment.
@@ -113,8 +116,16 @@ def self.wrap(env, status, headers, body, input = nil, head = false)
 			# @returns [Proc] A callback that calls all response finished handlers.
 			def self.completion_callback(response_finished, env, status, headers)
 				proc do |error|
-					response_finished.each do |callback|
-						callback.call(env, status, headers, error)
+					# Invoke callbacks in reverse order of registration, as specified by the Rack specification.
+					response_finished.reverse_each do |callback|
+						begin
+							callback.call(env, status, headers, error)
+						rescue => callback_error
+							# If a callback raises an exception, log it but continue invoking other callbacks.
+							# The Rack specification states that callbacks should not raise exceptions, but we handle
+							# this gracefully to prevent one misbehaving callback from breaking others.
+							Console.error(self, "Error occurred during response finished callback:", callback_error)
+						end
 					end
 				end
 			end

releases.md

@@ -1,5 +1,10 @@
 # Releases
 
+## Unreleased
+
+  - Correctly invoke `rack.response_finished` in reverse order.
+  - Tolerate errors during `rack.response_finished` callbacks.
+
 ## v0.17.0
 
   - Support `rack.response_finished` in Rack 2 if it's present in the environment.

test/protocol/rack/adapter/generic.rb

@@ -144,5 +144,66 @@
 			expect(response.status).to be == 500
 			expect(response.read).to be == "StandardError: Test error"
 		end
+		
+		it "invokes callbacks in reverse order of registration on error" do
+			call_order = []
+			
+			callback1 = proc do |env, status, headers, error|
+				call_order << 1
+			end
+			
+			callback2 = proc do |env, status, headers, error|
+				call_order << 2
+			end
+			
+			callback3 = proc do |env, status, headers, error|
+				call_order << 3
+			end
+			
+			app_with_callbacks = proc do |env|
+				env[Protocol::Rack::RACK_RESPONSE_FINISHED] = [callback1, callback2, callback3]
+				raise StandardError.new("Test error")
+			end
+			
+			adapter_with_callbacks = subject.new(app_with_callbacks)
+			response = adapter_with_callbacks.call(request)
+			
+			# Callbacks should be invoked in reverse order: 3, 2, 1
+			expect(call_order).to be == [3, 2, 1]
+			expect(response.status).to be == 500
+		end
+		
+		it "handles errors from callbacks gracefully and continues invoking other callbacks" do
+			call_order = []
+			
+			callback1 = proc do |env, status, headers, error|
+				call_order << 1
+			end
+			
+			callback2 = proc do |env, status, headers, error|
+				call_order << 2
+				raise StandardError.new("Callback error")
+			end
+			
+			callback3 = proc do |env, status, headers, error|
+				call_order << 3
+			end
+			
+			app_with_callbacks = proc do |env|
+				env[Protocol::Rack::RACK_RESPONSE_FINISHED] = [callback1, callback2, callback3]
+				raise StandardError.new("Test error")
+			end
+			
+			adapter_with_callbacks = subject.new(app_with_callbacks)
+			
+			expect(Console).to receive(:error).with(adapter_with_callbacks, "Error occurred during response finished callback:", instance_of(StandardError))
+			
+			response = adapter_with_callbacks.call(request)
+			
+			# All callbacks should be invoked despite callback2 raising an error
+			# Callbacks should be invoked in reverse order: 3, 2, 1
+			expect(call_order).to be == [3, 2, 1]
+			expect(response.status).to be == 500
+		end
 	end
 end

test/protocol/rack/body.rb

@@ -155,6 +155,87 @@ def body.each
 				expect(body).to be_nil
 				expect(called).to be == true
 			end
+			
+			it "invokes callbacks in reverse order of registration" do
+				call_order = []
+				
+				callback1 = proc do |env, status, headers, error|
+					call_order << 1
+				end
+				
+				callback2 = proc do |env, status, headers, error|
+					call_order << 2
+				end
+				
+				callback3 = proc do |env, status, headers, error|
+					call_order << 3
+				end
+				
+				env[Protocol::Rack::RACK_RESPONSE_FINISHED] = [callback1, callback2, callback3]
+				
+				body = subject.wrap(env, 200, headers, ["body"])
+				body.close
+				
+				# Callbacks should be invoked in reverse order: 3, 2, 1
+				expect(call_order).to be == [3, 2, 1]
+			end
+			
+			it "handles errors from callbacks gracefully and continues invoking other callbacks" do
+				call_order = []
+				
+				callback1 = proc do |env, status, headers, error|
+					call_order << 1
+				end
+				
+				callback2 = proc do |env, status, headers, error|
+					call_order << 2
+					raise StandardError.new("Callback error")
+				end
+				
+				callback3 = proc do |env, status, headers, error|
+					call_order << 3
+				end
+				
+				env[Protocol::Rack::RACK_RESPONSE_FINISHED] = [callback1, callback2, callback3]
+				
+				expect(Console).to receive(:error).with(Protocol::Rack::Body, "Error occurred during response finished callback:", instance_of(StandardError))
+				
+				body = subject.wrap(env, 200, headers, ["body"])
+				body.close
+				
+				# All callbacks should be invoked despite callback2 raising an error
+				# Callbacks should be invoked in reverse order: 3, 2, 1
+				expect(call_order).to be == [3, 2, 1]
+			end
+			
+			it "handles errors from callbacks when body is empty" do
+				call_order = []
+				
+				callback1 = proc do |env, status, headers, error|
+					call_order << 1
+				end
+				
+				callback2 = proc do |env, status, headers, error|
+					call_order << 2
+					raise StandardError.new("Callback error")
+				end
+				
+				callback3 = proc do |env, status, headers, error|
+					call_order << 3
+				end
+				
+				env[Protocol::Rack::RACK_RESPONSE_FINISHED] = [callback1, callback2, callback3]
+				
+				expect(Console).to receive(:error).with(Protocol::Rack::Body, "Error occurred during response finished callback:", instance_of(StandardError))
+				
+				body = subject.wrap(env, 204, headers, [])
+				
+				expect(body).to be_nil
+				
+				# All callbacks should be invoked despite callback2 raising an error
+				# Callbacks should be invoked in reverse order: 3, 2, 1
+				expect(call_order).to be == [3, 2, 1]
+			end
 		end
 	end
 end