fix: keep shared HTTP/2 connections pool-owned - #937
Open
smartinio wants to merge 2 commits into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After upgrading Hackney from v1 to v4, we immediately started seeing concurrent production requests fail with {error, closed} while another request on the same connection completed successfully.
We traced this to v4 negotiating HTTP/2 by default and the pooled connection remaining owned by the process that created it. When a synchronous requester exits, the shared connection exits with it and interrupts other requests using its streams.
This PR transfers ownership to the pool before publishing a connection for reuse. Registration keeps an existing ready connection, replaces an unusable one, and lets a busy one drain before it stops.
Connection probes, ownership transfers, and candidate shutdowns are bounded. Registration carries a deadline derived from checkout_timeout, falling back to connect_timeout, so expired registrations are rejected even if they reach the pool later. Failed, expired, and duplicate candidates are stopped rather than exposed with uncertain ownership, allowing their per-host slots to be released.
A request waiting for a per-host slot now rechecks whether a busy HTTP/2 connection has become reusable without exceeding the existing checkout timeout. Each HTTP/2 stream also tracks its requester, so an abandoned streaming upload or response in {async, once} mode is cancelled and cannot prevent a replaced connection from retiring.
The first commit adds regression coverage for connection ownership, registration races and cleanup, reuse and retirement of busy HTTP/2 connections, checkout deadlines, and abandoned streams. The second commit contains the implementation.