Ensure requests_active is decremented on error.

Author: samuel-williams-shopify

lib/falcon/body/request_finished.rb

@@ -14,7 +14,11 @@ module Body
 		# then decrements the metric. Use this so requests_active stays elevated until
 		# the request is fully finished (including response_finished callbacks).
 		class RequestFinished < Protocol::HTTP::Body::Wrapper
-			# Wrap a response body with a metric. If the body is nil or empty, decrements immediately.
+			# Wrap a response body with a metric.
+			#
+			# Expects the caller to have incremented `requests_active` beforehand. If the body is
+			# not wrapped (nil or empty message/body, or an error while assigning the wrapper), the
+			# `ensure` block decrements once. When wrapped, the metric is decremented in {#close}.
 			#
 			# @parameter message [Protocol::HTTP::Response] The response whose body to wrap.
 			# @parameter metric [Async::Utilization::Metric] The metric to decrement when the body is closed.
@@ -29,6 +33,8 @@ def self.wrap(message, metric)
 				message
 			end
 
+			# Initialize the wrapper.
+			#
 			# @parameter body [Protocol::HTTP::Body::Readable] The body to wrap.
 			# @parameter metric [Async::Utilization::Metric] The metric to decrement on close.
 			def initialize(body, metric)

lib/falcon/server.rb

@@ -69,10 +69,22 @@ def accept(...)
 		# Body::RequestFinished wrapper runs the decrement after the body closes,
 		# so response_finished callbacks are counted as active.
 		def call(...)
+			decrement = false
+			
 			@requests_total_metric.increment
 			@requests_active_metric.increment
+			decrement = true
+			
+			# Roll back `requests_active` unless the full `super` + wrap path completes. Covers
+			# `super` raising before `wrap` runs, errors inside `wrap`, and other abnormal exits.
+			response = Body::RequestFinished.wrap(super, @requests_active_metric)
+			decrement = false
 
-			return Body::RequestFinished.wrap(super, @requests_active_metric)
+			return response
+		ensure
+			if decrement
+				@requests_active_metric.decrement
+			end
 		end
 
 		# Generates a human-readable string representing the current statistics.

releases.md

@@ -1,8 +1,12 @@
 # Releases
 
+## Unreleased
+
+  - Decrement `requests_active` in `Falcon::Server#call` when `super` or `Falcon::Body::RequestFinished.wrap` raises, so utilization metrics are not leaked on error paths.
+
 ## v0.55.2
 
-  - Remove unnecessary require for `async/service/supervisor/supervised`."
+  - Remove unnecessary require for `async/service/supervisor/supervised`.
 
 ## v0.55.1
 

test/falcon/body/request_finished.rb

@@ -69,9 +69,9 @@
 		end
 
 		with "nil message" do
-			it "decrements immediately" do
+			it "decrements immediately and returns nil" do
 				metric.increment
-				subject.wrap(nil, metric)
+				expect(subject.wrap(nil, metric)).to be == nil
 				expect(metric.value).to be == 0
 			end
 		end

test/falcon/server.rb

@@ -148,5 +148,26 @@ def make_server(endpoint)
 			expect(utilization_registry.metric(:requests_active).value).to be == 0
 			expect(utilization_registry.metric(:requests_total).value).to be == 1
 		end
+		
+		it "restores requests_active when delegate raises" do
+			boom = Object.new
+			def boom.call(_request)
+				raise RuntimeError, "boom"
+			end
+			def boom.close
+			end
+			
+			endpoint = Async::HTTP::Endpoint.parse("http://127.0.0.1:0", reuse_port: true)
+			server = ::Falcon::Server.new(
+				::Protocol::HTTP::Middleware.new(boom),
+				endpoint,
+				utilization_registry: utilization_registry
+			)
+			
+			request = ::Protocol::HTTP::Request["GET", "/", {}]
+			expect{server.call(request)}.to raise_exception(RuntimeError, message: be =~ /boom/)
+			expect(utilization_registry.metric(:requests_active).value).to be == 0
+			expect(utilization_registry.metric(:requests_total).value).to be == 1
+		end
 	end
 end