feat(ProxyAgent) improve Curl-y behavior in HTTP->HTTP Proxy connections (#4180)#4340
feat(ProxyAgent) improve Curl-y behavior in HTTP->HTTP Proxy connections (#4180)#4340mcollina merged 3 commits intonodejs:mainfrom
Conversation
metcoder95
left a comment
There was a problem hiding this comment.
The changes lgtm, and I'm aligned with them; tho this seems like a breaking change that might be better to keep disabled by default and change it once in the next major
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:
|
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-v6.x v6.x
# Navigate to the new working tree
cd .worktrees/backport-v6.x
# Create a new branch
git switch --create backport-4340-to-v6.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b7513d4df62e9d1c1ecc34c3a418bd402e3c8432
# Push it to GitHub
git push --set-upstream origin backport-4340-to-v6.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-v6.xThen, create a pull request where the |
…ons (#4180) (#4340) * feat(ProxyAgent) improve Curl-y behavior in HTTP->HTTP Proxy connections (#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. * add a test to attempt multiple concurrent connections with a single HttpContext * be explicit about proxyTunnel status in each ProxyAgent test (cherry picked from commit b7513d4)
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-v6.x v6.x
# Navigate to the new working tree
cd .worktrees/backport-v6.x
# Create a new branch
git switch --create backport-4340-to-v6.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b7513d4df62e9d1c1ecc34c3a418bd402e3c8432
# Push it to GitHub
git push --set-upstream origin backport-4340-to-v6.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-v6.xThen, create a pull request where the |
…ons (nodejs#4180) (nodejs#4340) * feat(ProxyAgent) improve Curl-y behavior in HTTP->HTTP Proxy connections (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. * add a test to attempt multiple concurrent connections with a single HttpContext * be explicit about proxyTunnel status in each ProxyAgent test
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