fix: avoid premature cleanup of dispatcher in Agent#5034
fix: avoid premature cleanup of dispatcher in Agent#5034bienzaaron wants to merge 4 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]) { |
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.03% -0.01%
==========================================
Files 110 110
Lines 35793 35791 -2
==========================================
- Hits 33301 33297 -4
- Misses 2492 2494 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 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