Skip to content

[v7.x] fix(agent): keep pools that still have work when a connection closes - #5916

Open
SulimanAbdulrazzaq wants to merge 1 commit into
nodejs:v7.xfrom
SulimanAbdulrazzaq:fix/agent-pool-reuse-v7
Open

SulimanAbdulrazzaq wants to merge 1 commit into
nodejs:v7.xfrom
SulimanAbdulrazzaq:fix/agent-pool-reuse-v7

Conversation

@SulimanAbdulrazzaq

Copy link
Copy Markdown

This relates to...

Fixes #5910 on v7.x (it is also the v7.x form of #5022). Node.js 24 bundles undici 7.x, so its global fetch is affected.

Rationale

On v7.x, Agent counts connect and disconnect events for each origin key and closes the pool when the count reaches 0. When a server ends a keep-alive socket with Connection: close:

  1. The disconnect can bring the count to 0 while the next request is already queued on that pool. The pool is closed, so it serves that one request and then goes away.
  2. That pool's own disconnect arrives later. It looks up the key, finds the pool that replaced it, and decrements the replacement's count. The replacement is closed after one request too, and every later connection serves a single request.

With the reproducer from #5910 (server closes each socket after 100 requests, 400 sequential fetches), the sockets served [100,100,1,1,1,1,1,1] on v7.x.

On main this logic was already replaced (#5034, then kPending in #5740). This PR ports the same decision rule to v7.x.

Changes

Bug Fixes

  • lib/dispatcher/agent.js: closeClientIfUnused now decides from the pool's own state instead of the shared counter:
    • it ignores events from a pool that was already replaced under the same key;
    • it keeps a pool while kConnected > 0, kBusy or kPending > 0.
  • The { dispatcher } entry shape is kept because MockAgent shares this map on v7.x. Only the count field is dropped.
  • test/agent-connection-management.js (new): the server closes each socket after 3 requests, and the tests send 12 sequential requests through an Agent, one test with request() and one with fetch(). Both expect [3, 3, 3, 3] requests per socket.

Not included: the maxRequestsPerClient counter change from #5034 in client-h1.js, because it is a separate fix.

Breaking Changes and Deprecations

N/A

Status

Tested on Node 24:

Agent counted connect and disconnect events per origin key and closed
the pool when the count reached zero. When a server ends a keep-alive
socket with Connection: close, the disconnect can drop the count to zero
while the next request is already queued on that pool, so the pool is
closed after serving one more request. Its late disconnect then
decrements the count of the pool that replaced it under the same key,
which closes that pool too, and every later connection serves a single
request.

Decide from the pool's own state instead, as main does: ignore events
from a pool that was already replaced, and keep a pool while it has
connected clients, is busy, or has pending requests.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.12%. Comparing base (0c74bec) to head (8f15d63).

Additional details and impacted files
@@           Coverage Diff           @@
##             v7.x    #5916   +/-   ##
=======================================
  Coverage   93.11%   93.12%           
=======================================
  Files         112      112           
  Lines       37366    37371    +5     
=======================================
+ Hits        34793    34800    +7     
+ Misses       2573     2571    -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This branch has not been deployed

No deployments
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