fix: avoid premature cleanup of dispatcher in Agent#5034
fix: avoid premature cleanup of dispatcher in Agent#5034bienzaaron wants to merge 7 commits intonodejs:mainfrom
Conversation
| if (dispatcher[kConnected] > 0 || dispatcher[kBusy]) { | ||
| return | ||
| } |
There was a problem hiding this comment.
The key difference is here - we check active connections as well as kBusy:
undici/lib/dispatcher/client.js
Lines 326 to 332 in bc0a19c
If we have either active connections or the dispatcher is busy, we don't close it.
This lets us clean up the connection-tracking bookkeeping and just use information already in the dispatcher.
| this[kOrigins].delete(origin) | ||
| let hasOrigin = false | ||
| for (const client of this[kClients].values()) { | ||
| if (client[kUrl].origin === dispatcher[kUrl].origin) { |
There was a problem hiding this comment.
slightly updated logic here, again to reduce bookkeeping -- access origin from client[kUrl].origin instead of storing it
| } | ||
|
|
||
| if (client[kMaxRequests] && socket[kCounter]++ >= client[kMaxRequests]) { | ||
| if (client[kMaxRequests] && ++socket[kCounter] >= client[kMaxRequests]) { |
There was a problem hiding this comment.
That change is covered by your tests?
There was a problem hiding this comment.
Yes, it is covered by the assertion here - https://github.com/nodejs/undici/pull/5034/changes#diff-aa5d6e637dbf3a57f70e146170bcc8917253e98f0885ff7d1d1875936a60255eR116-R117
but I fixed some assertions in test/close-and-destroy.js as well in 5d75f78 that were masking this issue before. They only tested the connection count overall, not where the boundary between connections are.
bienzaaron
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5034 +/- ##
==========================================
+ Coverage 93.03% 93.10% +0.06%
==========================================
Files 110 110
Lines 35793 35803 +10
==========================================
+ Hits 33301 33335 +34
+ Misses 2492 2468 -24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @@ -62,4 +62,96 @@ describe('Agent should close inactive clients', () => { | |||
|
|
|||
| await p | |||
| }) | |||
|
|
|||
There was a problem hiding this comment.
the file is named after a specific issue. Given that you are adding more things, can you pick a better name?
| p = new Promise((resolve, reject) => { | ||
| _resolve = resolve | ||
| _reject = reject | ||
| }) |
There was a problem hiding this comment.
You can use Promise.withResolvers()
There was a problem hiding this comment.
These were existing tests from test/issue-4244.js, but cleaned up in d897e6e
| } | ||
|
|
||
| if (client[kMaxRequests] && socket[kCounter]++ >= client[kMaxRequests]) { | ||
| if (client[kMaxRequests] && ++socket[kCounter] >= client[kMaxRequests]) { |
There was a problem hiding this comment.
That change is covered by your tests?
4 participants
This relates to...
#5022
Changes
The fixes in #4223 and #4425 introduce a bug where keep-alive connections are sometimes not correctly reused after the first disconnection for a dispatcher.
The issue is a race between disconnect and new dispatches. After a response with
Connection: closeor after the connection reaches its request limit, the socket disconnects. If a new request has already been dispatched to the same dispatcher but has not yet been processed into a new connection, thedisconnecthandler can incorrectly treat the dispatcher as unused and callclose()on it.As far as I can tell, this is still a graceful failure (new request doesn't fail), but it does result in a new connection being used. If new requests are continually dispatched like this, then connections are never reused after the first one disconnects. Delaying the next dispatched request to the next event loop iteration (e.g. via
setTimeout(cb, 0)) or longer results in the connection being reused correctly.Features
N/A
Bug Fixes
#5022
Breaking Changes and Deprecations
N/A
Status