WIP: Max total connections support for BalancedPool #4192
Conversation
| return this[kClients].map(pool => pool[kConnected]).reduce((a, b) => a + b, 0) | ||
| } | ||
|
|
||
| [kQueueResume] () { |
There was a problem hiding this comment.
@metcoder95 You asked elsewhere why I overrode other queue resume code. For one, I couldn't see how other extensions of PoolBase or DispatcherBase were triggering a queue resume event; if you can point me to where/how that happens that would be fantastic.
But also, this implementation specifically optimizes for finding an available Pool with a matching origin.
Thanks for your patience, as this is my first time digging through the Undici codebase, so I'm learning the internals as I go.
There was a problem hiding this comment.
But is there a specific reason to override it? Or why was required? (maybe I missed something before)
There was a problem hiding this comment.
In my initial test runs, without this force trigger, the queue was never resumed and all pending requests were never run.
| const { onComplete } = handler | ||
| handler.onComplete = (...args) => { | ||
| setTimeout(this[kQueueResume](), 0) | ||
| if (onComplete) onComplete(...args) |
There was a problem hiding this comment.
I mentioned this in #3268, but to put it in context: wrapping a handler's onComplete function appears to happen every time a request is attempted, so if a request is taken from the queue x times before finally being sent, this call to kQueueResume happens x times as well. Testing this locally I noted that it caused kQueueResume calls to increase exponentially over time, which is not at all ideal.
I tried adding a private symbol handler[kResumable] = true when it had been wrapped, to avoid re-wrapping, but it seems that handler is a brand new object every time kDispatch is called.
| if (typeof factory !== 'function') { | ||
| throw new InvalidArgumentError('factory must be a function.') | ||
| } | ||
|
|
||
| if (maxTotalConnections != null && (!Number.isFinite(maxTotalConnections) || maxTotalConnections < 0)) { |
There was a problem hiding this comment.
For backwards compatibility, shouldn't be better to default it to Infinity?
| return this[kClients].map(pool => pool[kConnected]).reduce((a, b) => a + b, 0) | ||
| } | ||
|
|
||
| [kQueueResume] () { |
There was a problem hiding this comment.
But is there a specific reason to override it? Or why was required? (maybe I missed something before)
This relates to...
#3268 (comment)
Rationale
Work in progress on being able to enforce maximum total connections across all origins in a
BalancedPoolandAgent.Changes
Features
Exposes
maxTotalConnectionsoption onBalancedPoolandAgentthat ensures the total number of open connections across all origins does not exceed the configured maximum.Bug Fixes
Breaking Changes and Deprecations
Status