Skip to content

perf: use Pool instead of Client in ProxyAgent for HTTP proxy connections #4359

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mcollina wants to merge 1 commit into from
Closed

perf: use Pool instead of Client in ProxyAgent for HTTP proxy connections #4359

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions lib/dispatcher/proxy-agent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
'use strict'

const { kProxy, kClose, kDestroy, kDispatch, kConnector } = require('../core/symbols')
const { kProxy, kClose, kDestroy, kDispatch } = require('../core/symbols')
const { URL } = require('node:url')
const Agent = require('./agent')
const Pool = require('./pool')
const DispatcherBase = require('./dispatcher-base')
const { InvalidArgumentError, RequestAbortedError, SecureProxyConnectionError } = require('../core/errors')
const buildConnector = require('../core/connect')
const Client = require('./client')

const kAgent = Symbol('proxy agent')
const kClient = Symbol('proxy client')
Expand All @@ -29,6 +28,7 @@ const noop = () => {}

class ProxyClient extends DispatcherBase {
#client = null
#connector = null
constructor (origin, opts) {
if (typeof origin === 'string') {
origin = new URL(origin)
Expand All @@ -40,7 +40,9 @@ class ProxyClient extends DispatcherBase {

super()

this.#client = new Client(origin, opts)
// Store the connector for direct socket connections
this.#connector = opts?.connect || buildConnector(opts)
this.#client = new Pool(origin, opts)
Copy link
Copy Markdown
Contributor

@caitp caitp Jul 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new Http1ProxyWrapper does this, although looking at the diff it's kind of clumbsy how it does it -- but the default factory it uses will make a Pool unless opts.connections === 1, which is pulled from the regular Agent factory.

I did switch to using a user supplied factory in order to get MockAgent tests passing, though. maybe it is a good simplification to just always create a Pool, though

Copy link
Copy Markdown
Member Author

@mcollina mcollina Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then this PR is not needed

}

async [kClose] () {
Expand All @@ -54,7 +56,7 @@ class ProxyClient extends DispatcherBase {
async [kDispatch] (opts, handler) {
const { method, origin } = opts
if (method === 'CONNECT') {
this.#client[kConnector]({
this.#connector({
origin,
port: opts.port || defaultProtocolPort(opts.protocol),
path: opts.host,
Expand Down Expand Up @@ -121,7 +123,7 @@ class ProxyAgent extends DispatcherBase {
if (origin.protocol === 'http:') {
return new ProxyClient(origin, options)
}
return new Client(origin, options)
return new Pool(origin, options)
}
: undefined

Expand Down
Loading