feat(ProxyAgent) improve Curl-y behavior in HTTP->HTTP Proxy connections (#4180) #4340
Conversation
lib/dispatcher/proxy-agent.js
Outdated
| // FIXME: remove wrapper from the pool | ||
| } | ||
|
|
||
| async [kDestroy] () { | ||
| await this.#client.destroy() | ||
| // FIXME: remove wrapper from the pool |
There was a problem hiding this comment.
why not close the this.#client?
There was a problem hiding this comment.
as noted in the PR description, this.#client is currently the original Proxy client -- which I think is actually a huge problem, but I haven't written a test yet to demonstrate that it is, which is probably next up. Maybe it's actually fine to reuse the same HttpContext for concurrent requests, but I suspect it isn't
But given that it is the Proxy server client, closing it breaks the ProxyAgent
|
I've attempted to add a test for this reuse of the HttpContext, and haven't been able to reproduce any issues yet (although the test case is pretty small) -- so maybe it actually does work? but still need to look into the other CI failures. |
…ons (nodejs#4180) This refactors the way the legacy unsecured behaviour is implemented, by wrapping the Proxy client in a wrapper which rewrites requests, and handles errors. This will also insert authentication headers in each request.
|
summary of the changes:
|
This refactors the way the legacy unsecured behaviour is implemented, by wrapping the Proxy client in a wrapper which rewrites requests, and handles errors. This will also insert authentication headers in each request.
CURRENTLY:
For nonsecure channels, clients allocated by the pool all delegate to the proxy client, and reuse the same HttpContext. This is probably not a good thing, but at least in cases where only one request is performed at a time, it seems harmless enough. In order to make this ready to land, this probably needs to handle simultaneous concurrent requests and have a test for that.
This relates to...
Rationale
Changes
Features
Bug Fixes
Breaking Changes and Deprecations
Status