Skip to content

Fix after_request_complete getting nil env #179

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hmcguire-shopify wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix after_request_complete getting nil env #179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/pitchfork/http_server.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,7 @@ def process_client(client, worker, timeout_handler)
rescue => application_error
handle_error(client, application_error, response_written)
env
env || @request&.env
ensure
if env
env["rack.response_finished"].reverse_each do |callback|
Expand Down Expand Up @@ -1016,7 +1017,7 @@ def worker_loop(worker)
else
request_env = process_client(client, worker, prepare_timeout(worker))
worker.increment_requests_count
@after_request_complete&.call(self, worker, request_env)
@after_request_complete&.call(self, worker, request_env || {})
Copy link
Contributor

@byroot byroot Dec 5, 2025

Choose a reason for hiding this comment

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

Should we just skip the callback instead?

I expect many callback would fail on an empty hash about as much as on nil, because some mandatory keys (e.g. PATH_INFO would be missing).

Technically, if we failed to parse the request, there's no "after request" stage.

Copy link
Member

@matthewd matthewd Dec 8, 2025

Choose a reason for hiding this comment

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

Yeah, that's fair.

IMO it's probably worth doing the @request&.env "saving attempt" -- because malformed or not, we did receive a request, so the callback-author probably wants to know about it. If we've failed parsing the request it's clearly not going to be complete, but there's still a decent chance it has a useful amount of context.

Copy link
Contributor Author

@hmcguire-shopify hmcguire-shopify Dec 8, 2025

Choose a reason for hiding this comment

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

Noting things we use it for:

  • worker_requests_count metric
  • OOBGC
  • OOMKiller

Since the worker's request_count is incremented on the line above I feel like it makes sense to continue allowing applications to emit that (although we could also make that conditional on the request parsing). The other two things seem less important for a request that fails to parse

end

worker.update_deadline(@timeout)
Expand Down
24 changes: 24 additions & 0 deletions test/integration/test_parser_error.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,4 +121,28 @@ def test_non_ascii_headers

assert_clean_shutdown(pid)
end

def test_after_request_complete_receives_partial_env_on_error
addr, port = unused_port

pid = spawn_server(app: File.join(ROOT, "test/integration/env.ru"), config: <<~CONFIG)
listen "#{addr}:#{port}"

after_request_complete do |server, worker, env|
$stderr.puts "[after_request_complete] \#{"BAD" unless env['REQUEST_METHOD'] == "GET"}"
end
CONFIG

assert_healthy("http://#{addr}:#{port}")

# Trigger parse error
Socket.tcp(addr, port) do |sock|
sock.print("BAD")
sock.close_write
end

assert_stderr("[after_request_complete] BAD")

assert_clean_shutdown(pid)
end
end