fix: track origins by client map key in Agent teardown#5537
Open
emmayusufu wants to merge 1 commit into
Open
Conversation
closeClientIfUnused read client[kUrl].origin for every dispatcher in the client map, but dispatchers produced by a custom factory, including the Http1ProxyWrapper and Socks5ProxyAgent that ProxyAgent registers, never set kUrl, so plain-http requests through a proxy crashed on the disconnect and connectionError teardown path. Compare the map keys against the origin string captured at dispatch time instead, which also makes the kOrigins delete symmetric with the add when opts.origin is a URL instance. Signed-off-by: Emmanuel Yusufu Kimaswa <kimaswaemma36@gmail.com>
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
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.
This relates to...
Fixes #5529
Rationale
Agent#closeClientIfUnusedreadsclient[kUrl].originfor every dispatcher in the client map, but the factory option is public API and nothing guarantees a factory-built dispatcher setskUrl.ProxyAgent's own factory registersHttp1ProxyWrapper(andSocks5ProxyAgent), neither of which does, so since 8.7.0 a plain-http request through a proxy crashes withCannot read properties of undefined (reading 'origin')on the disconnect / connectionError teardown path.Changes
The teardown closure already holds the
originstring that was added tokOriginsand the mapkeyderived from it, so the bookkeeping doesn't needkUrlat all: check the remaining map keys for the origin (including the#http1-onlyvariant) and delete the capturedoriginstring. This is the third option suggested in the issue.A side effect worth noting:
kOrigins.deleteis now symmetric with theadd. PreviouslyaddusedString(opts.origin)whiledeleteusedkUrl.origin, which diverge whenopts.originis aURLinstance (trailing slash), so those origins were never removed and counted againstmaxOriginsforever.The
statsgetter still readsdispatcher[kUrl].originand has the same fragility with kUrl-less dispatchers; left alone here since changing its keys is a visible behavior change. Can follow up if there's interest.Features
N/A
Bug Fixes
ProxyAgentno longer crashes the innerAgenton connection teardown for plain-http (non-tunneled) targets.Breaking Changes and Deprecations
N/A
Status