Add clientFactory option to ProxyAgent (nodejs#2003)

* Add clientFactory option to ProxyAgent

The default use of a Client means that HTTP CONNECT requests to the
Proxy will block successive requests. Giving consumers the option to
supply a factory allows them use a Pool which ensures that these
requests will not block.

fixes: nodejs#2001

* fixup! Use pool by default and change clientFactory return type

* Add clientFactory to ProxyAgent docs

---------

Co-authored-by: Andrew Fecenko <afecenko@atlassian.com>

docs/api/ProxyAgent.md

@@ -19,6 +19,7 @@ Extends: [`AgentOptions`](Agent.md#parameter-agentoptions)
 * **uri** `string` (required) - It can be passed either by a string or a object containing `uri` as string.
 * **token** `string` (optional) - It can be passed by a string of token for authentication.
 * **auth** `string` (**deprecated**) - Use token.
+* **clientFactory** `(origin: URL, opts: Object) => Dispatcher` - Default: `(origin, opts) => new Pool(origin, opts)`
 
 Examples:
 

lib/proxy-agent.js

@@ -3,7 +3,7 @@
 const { kProxy, kClose, kDestroy, kInterceptors } = require('./core/symbols')
 const { URL } = require('url')
 const Agent = require('./agent')
-const Client = require('./client')
+const Pool = require('./pool')
 const DispatcherBase = require('./dispatcher-base')
 const { InvalidArgumentError, RequestAbortedError } = require('./core/errors')
 const buildConnector = require('./core/connect')
@@ -34,6 +34,10 @@ function buildProxyOptions (opts) {
   }
 }
 
+function defaultFactory (origin, opts) {
+  return new Pool(origin, opts)
+}
+
 class ProxyAgent extends DispatcherBase {
   constructor (opts) {
     super(opts)
@@ -51,6 +55,12 @@ class ProxyAgent extends DispatcherBase {
       throw new InvalidArgumentError('Proxy opts.uri is mandatory')
     }
 
+    const { clientFactory = defaultFactory } = opts
+
+    if (typeof clientFactory !== 'function') {
+      throw new InvalidArgumentError('Proxy opts.clientFactory must be a function.')
+    }
+
     this[kRequestTls] = opts.requestTls
     this[kProxyTls] = opts.proxyTls
     this[kProxyHeaders] = opts.headers || {}
@@ -69,7 +79,7 @@ class ProxyAgent extends DispatcherBase {
 
     const connect = buildConnector({ ...opts.proxyTls })
     this[kConnectEndpoint] = buildConnector({ ...opts.requestTls })
-    this[kClient] = new Client(resolvedUrl, { connect })
+    this[kClient] = clientFactory(resolvedUrl, { connect })
     this[kAgent] = new Agent({
       ...opts,
       connect: async (opts, callback) => {

test/proxy-agent.js

@@ -7,6 +7,7 @@ const { nodeMajor } = require('../lib/core/util')
 const { readFileSync } = require('fs')
 const { join } = require('path')
 const ProxyAgent = require('../lib/proxy-agent')
+const Pool = require('../lib/pool')
 const { createServer } = require('http')
 const https = require('https')
 const proxy = require('proxy')
@@ -72,6 +73,45 @@ test('use proxy-agent to connect through proxy', async (t) => {
   proxyAgent.close()
 })
 
+test('use proxy agent to connect through proxy using Pool', async (t) => {
+  t.plan(3)
+  const server = await buildServer()
+  const proxy = await buildProxy()
+  let resolveFirstConnect
+  let connectCount = 0
+
+  proxy.authenticate = async function (req, fn) {
+    if (++connectCount === 2) {
+      t.pass('second connect should arrive while first is still inflight')
+      resolveFirstConnect()
+      fn(null, true)
+    } else {
+      await new Promise((resolve) => {
+        resolveFirstConnect = resolve
+      })
+      fn(null, true)
+    }
+  }
+
+  server.on('request', (req, res) => {
+    res.end()
+  })
+
+  const serverUrl = `http://localhost:${server.address().port}`
+  const proxyUrl = `http://localhost:${proxy.address().port}`
+  const clientFactory = (url, options) => {
+    return new Pool(url, options)
+  }
+  const proxyAgent = new ProxyAgent({ auth: Buffer.from('user:pass').toString('base64'), uri: proxyUrl, clientFactory })
+  const firstRequest = request(`${serverUrl}`, { dispatcher: proxyAgent })
+  const secondRequest = await request(`${serverUrl}`, { dispatcher: proxyAgent })
+  t.equal((await firstRequest).statusCode, 200)
+  t.equal(secondRequest.statusCode, 200)
+  server.close()
+  proxy.close()
+  proxyAgent.close()
+})
+
 test('use proxy-agent to connect through proxy using path with params', async (t) => {
   t.plan(6)
   const server = await buildServer()

test/types/proxy-agent.test-d.ts

@@ -1,6 +1,6 @@
 import { expectAssignable } from 'tsd'
 import { URL } from 'url'
-import { ProxyAgent, setGlobalDispatcher, getGlobalDispatcher, Agent } from '../..'
+import { ProxyAgent, setGlobalDispatcher, getGlobalDispatcher, Agent, Pool } from '../..'
 
 expectAssignable<ProxyAgent>(new ProxyAgent(''))
 expectAssignable<ProxyAgent>(new ProxyAgent({ uri: '' }))
@@ -25,7 +25,8 @@ expectAssignable<ProxyAgent>(
       cert: '',
       servername: '',
       timeout: 1
-    }
+    },
+    clientFactory: (origin: URL, opts: object) => new Pool(origin, opts)
   })
 )
 

types/proxy-agent.d.ts

@@ -1,7 +1,9 @@
 import Agent from './agent'
 import buildConnector from './connector';
+import Client from './client'
 import Dispatcher from './dispatcher'
 import { IncomingHttpHeaders } from './header'
+import Pool from './pool'
 
 export default ProxyAgent
 
@@ -23,5 +25,6 @@ declare namespace ProxyAgent {
     headers?: IncomingHttpHeaders;
     requestTls?: buildConnector.BuildOptions;
     proxyTls?: buildConnector.BuildOptions;
+    clientFactory?(origin: URL, opts: object): Dispatcher;
   }
 }