Fix keepAlive for http-proxy-agent

.changeset/fair-swans-heal.md

@@ -0,0 +1,7 @@
+---
+'http-proxy-agent': patch
+'pac-proxy-agent': patch
+'proxy-agent': patch
+---
+
+The keepAlive option now works for HTTP proxies

packages/agent-base/src/index.ts

@@ -5,18 +5,6 @@ import { Duplex } from 'stream';
 
 export * from './helpers';
 
-function isSecureEndpoint(): boolean {
-	const { stack } = new Error();
-	if (typeof stack !== 'string') return false;
-	return stack
-		.split('\n')
-		.some(
-			(l) =>
-				l.indexOf('(https.js:') !== -1 ||
-				l.indexOf('node:https:') !== -1
-		);
-}
-
 interface HttpConnectOpts extends net.TcpNetConnectOpts {
 	secureEndpoint: false;
 }
@@ -28,6 +16,15 @@ interface HttpsConnectOpts extends tls.ConnectionOptions {
 
 export type AgentConnectOpts = HttpConnectOpts | HttpsConnectOpts;
 
+/** Platform-provided ClientRequest type with internal fields exposed */
+export interface InternalClientRequest extends http.ClientRequest {
+	outputData?: {
+		data: string;
+	}[];
+	_header?: string | null;
+	_implicitHeader(): void;
+}
+
 export abstract class Agent extends http.Agent {
 	_defaultPort?: number;
 	_protocol?: string;
@@ -48,14 +45,26 @@ export abstract class Agent extends http.Agent {
 		options: AgentConnectOpts
 	): Promise<Duplex | http.Agent> | Duplex | http.Agent;
 
+	protected isSecureEndpoint(): boolean {
+		const { stack } = new Error();
+		if (typeof stack !== 'string') return false;
+		return stack
+			.split('\n')
+			.some(
+				(l) =>
+					l.indexOf('(https.js:') !== -1 ||
+					l.indexOf('node:https:') !== -1
+			);
+	}
+
 	createSocket(
 		req: http.ClientRequest,
 		options: AgentConnectOpts,
 		cb: (err: Error | null, s?: Duplex) => void
 	) {
 		const o = {
 			...options,
-			secureEndpoint: options.secureEndpoint ?? isSecureEndpoint(),
+			secureEndpoint: options.secureEndpoint ?? this.isSecureEndpoint(),
 		};
 		Promise.resolve()
 			.then(() => this.connect(req, o))
@@ -93,7 +102,7 @@ export abstract class Agent extends http.Agent {
 		if (typeof this._protocol === 'string') {
 			return this._protocol;
 		}
-		const p = isSecureEndpoint() ? 'https:' : 'http:';
+		const p = this.isSecureEndpoint() ? 'https:' : 'http:';
 		return p;
 	}
 

packages/agent-base/test/test.ts

@@ -462,6 +462,7 @@ describe('Agent (TypeScript)', () => {
 				assert(gotReq);
 				expect(connectCount).toEqual(1);
 				expect(res.headers.connection).toEqual('keep-alive');
+				expect(res.statusCode).toEqual(200);
 				res.resume();
 				const s1 = res.socket;
 
@@ -473,6 +474,7 @@ describe('Agent (TypeScript)', () => {
 				});
 				expect(connectCount).toEqual(1);
 				expect(res2.headers.connection).toEqual('keep-alive');
+				expect(res2.statusCode).toEqual(200);
 				assert(res2.socket === s1);
 
 				res2.resume();

packages/http-proxy-agent/src/index.ts

@@ -3,7 +3,7 @@ import * as tls from 'tls';
 import * as http from 'http';
 import createDebug from 'debug';
 import { once } from 'events';
-import { Agent, AgentConnectOpts } from 'agent-base';
+import { Agent, AgentConnectOpts, InternalClientRequest } from 'agent-base';
 
 const debug = createDebug('http-proxy-agent');
 
@@ -21,15 +21,18 @@ type ConnectOpts<T> = {
 		: never;
 }[keyof ConnectOptsMap];
 
-export type HttpProxyAgentOptions<T> = ConnectOpts<T> & http.AgentOptions;
+type RequestOptions = {
+	/**
+	 * If using HttpProxyAgent internally for a request that has already
+	 * been generated, this option allows request properties to be
+	 * rewritten during the onConnect() method.
+	 */
+	setRequestPropsOnConnect?: boolean;
+};
 
-interface HttpProxyAgentClientRequest extends http.ClientRequest {
-	outputData?: {
-		data: string;
-	}[];
-	_header?: string | null;
-	_implicitHeader(): void;
-}
+export type HttpProxyAgentOptions<T> = ConnectOpts<T> &
+	http.AgentOptions &
+	RequestOptions;
 
 function isHTTPS(protocol?: string | null): boolean {
 	return typeof protocol === 'string' ? /^https:?$/i.test(protocol) : false;
@@ -44,6 +47,7 @@ export class HttpProxyAgent<Uri extends string> extends Agent {
 
 	readonly proxy: URL;
 	connectOpts: net.TcpNetConnectOpts & tls.ConnectionOptions;
+	setRequestPropsOnConnect: boolean;
 
 	get secureProxy() {
 		return isHTTPS(this.proxy.protocol);
@@ -64,19 +68,16 @@ export class HttpProxyAgent<Uri extends string> extends Agent {
 			: this.secureProxy
 			? 443
 			: 80;
+		const { setRequestPropsOnConnect = false, ...rest } = opts || {};
 		this.connectOpts = {
-			...opts,
+			...rest,
 			host,
 			port,
 		};
+		this.setRequestPropsOnConnect = setRequestPropsOnConnect;
 	}
 
-	async connect(
-		req: HttpProxyAgentClientRequest,
-		opts: AgentConnectOpts
-	): Promise<net.Socket> {
-		const { proxy } = this;
-
+	setRequestProps(req: InternalClientRequest, opts: AgentConnectOpts): void {
 		const protocol = opts.secureEndpoint ? 'https:' : 'http:';
 		const hostname = req.getHeader('host') || 'localhost';
 		const base = `${protocol}//${hostname}`;
@@ -89,6 +90,8 @@ export class HttpProxyAgent<Uri extends string> extends Agent {
 		// to the absolute path of the URL that will be requested.
 		req.path = String(url);
 
+		const { proxy } = this;
+
 		// Inject the `Proxy-Authorization` header if necessary.
 		req._header = null;
 		if (proxy.username || proxy.password) {
@@ -107,7 +110,21 @@ export class HttpProxyAgent<Uri extends string> extends Agent {
 				this.keepAlive ? 'Keep-Alive' : 'close'
 			);
 		}
+	}
+
+	addRequest(req: InternalClientRequest, opts: AgentConnectOpts): void {
+		this.setRequestProps(req, opts);
+		// @ts-expect-error `addRequest()` isn't defined in `@types/node`
+		super.addRequest(req, opts);
+	}
 
+	async connect(
+		req: InternalClientRequest,
+		opts: AgentConnectOpts
+	): Promise<net.Socket> {
+		if (this.setRequestPropsOnConnect) {
+			this.setRequestProps(req, opts);
+		}
 		// Create a socket connection to the proxy server.
 		let socket: net.Socket;
 		if (this.secureProxy) {
@@ -118,23 +135,25 @@ export class HttpProxyAgent<Uri extends string> extends Agent {
 			socket = net.connect(this.connectOpts);
 		}
 
-		// At this point, the http ClientRequest's internal `_header` field
-		// might have already been set. If this is the case then we'll need
-		// to re-generate the string since we just changed the `req.path`.
-		let first: string;
-		let endOfHeaders: number;
-		debug('Regenerating stored HTTP header string for request');
-		req._implicitHeader();
-		if (req.outputData && req.outputData.length > 0) {
-			// Node >= 12
-			debug(
-				'Patching connection write() output buffer with updated header'
-			);
-			first = req.outputData[0].data;
-			endOfHeaders = first.indexOf('\r\n\r\n') + 4;
-			req.outputData[0].data =
-				req._header + first.substring(endOfHeaders);
-			debug('Output buffer: %o', req.outputData[0].data);
+		if (this.setRequestPropsOnConnect) {
+			// At this point, the http ClientRequest's internal `_header` field
+			// might have already been set. If this is the case then we'll need
+			// to re-generate the string since we just changed the `req.path`.
+			let first: string;
+			let endOfHeaders: number;
+			debug('Regenerating stored HTTP header string for request');
+			req._implicitHeader();
+			if (req.outputData && req.outputData.length > 0) {
+				// Node >= 12
+				debug(
+					'Patching connection write() output buffer with updated header'
+				);
+				first = req.outputData[0].data;
+				endOfHeaders = first.indexOf('\r\n\r\n') + 4;
+				req.outputData[0].data =
+					req._header + first.substring(endOfHeaders);
+				debug('Output buffer: %o', req.outputData[0].data);
+			}
 		}
 
 		// Wait for the socket's `connect` event, so that this `callback()`

packages/http-proxy-agent/test/test.ts

@@ -2,6 +2,7 @@ import * as fs from 'fs';
 import * as http from 'http';
 import * as https from 'https';
 import assert from 'assert';
+import { once } from 'events';
 import { createProxy, ProxyServer } from 'proxy';
 import { listen } from 'async-listen';
 import { json, req } from 'agent-base';
@@ -187,5 +188,32 @@ describe('HttpProxyAgent', () => {
 			const body = await json(res);
 			expect(body.host).toEqual(httpServerUrl.hostname);
 		});
+
+		it('should work with `keepAlive: true`', async () => {
+			httpServer.on('request', (req, res) => {
+				res.end(JSON.stringify(req.headers));
+			});
+
+			const agent = new HttpProxyAgent(proxyUrl, { keepAlive: true });
+
+			try {
+				const res = await req(httpServerUrl, { agent });
+				expect(res.headers.connection).toEqual('keep-alive');
+				expect(res.statusCode).toEqual(200);
+				res.resume();
+				const s1 = res.socket;
+				await once(s1, 'free');
+
+				const res2 = await req(httpServerUrl, { agent });
+				expect(res2.headers.connection).toEqual('keep-alive');
+				expect(res2.statusCode).toEqual(200);
+				res2.resume();
+				const s2 = res2.socket;
+				assert(s1 === s2);
+				await once(s2, 'free');
+			} finally {
+				agent.destroy();
+			}
+		});
 	});
 });

packages/https-proxy-agent/test/test.ts

@@ -215,12 +215,14 @@ describe('HttpsProxyAgent', () => {
 			try {
 				const res = await req(serverUrl, { agent });
 				expect(res.headers.connection).toEqual('keep-alive');
+				expect(res.statusCode).toEqual(200);
 				res.resume();
 				const s1 = res.socket;
 				await once(s1, 'free');
 
 				const res2 = await req(serverUrl, { agent });
 				expect(res2.headers.connection).toEqual('keep-alive');
+				expect(res2.statusCode).toEqual(200);
 				res2.resume();
 				const s2 = res2.socket;
 				assert(s1 === s2);
@@ -332,6 +334,7 @@ describe('HttpsProxyAgent', () => {
 					rejectUnauthorized: false,
 				});
 				expect(res.headers.connection).toEqual('keep-alive');
+				expect(res.statusCode).toEqual(200);
 				res.resume();
 				const s1 = res.socket;
 				await once(s1, 'free');
@@ -341,6 +344,7 @@ describe('HttpsProxyAgent', () => {
 					rejectUnauthorized: false,
 				});
 				expect(res2.headers.connection).toEqual('keep-alive');
+				expect(res2.statusCode).toEqual(200);
 				res2.resume();
 				const s2 = res2.socket;
 				assert(s1 === s2);

packages/pac-proxy-agent/src/index.ts

@@ -6,7 +6,12 @@ import { once } from 'events';
 import createDebug from 'debug';
 import { Readable } from 'stream';
 import { format } from 'url';
-import { Agent, AgentConnectOpts, toBuffer } from 'agent-base';
+import {
+	Agent,
+	AgentConnectOpts,
+	InternalClientRequest,
+	toBuffer,
+} from 'agent-base';
 import { HttpProxyAgent, HttpProxyAgentOptions } from 'http-proxy-agent';
 import { HttpsProxyAgent, HttpsProxyAgentOptions } from 'https-proxy-agent';
 import { SocksProxyAgent, SocksProxyAgentOptions } from 'socks-proxy-agent';
@@ -69,6 +74,7 @@ export class PacProxyAgent<Uri extends string> extends Agent {
 	resolver?: FindProxyForURL;
 	resolverHash: string;
 	resolverPromise?: Promise<FindProxyForURL>;
+	httpProxyAgent?: HttpProxyAgent<Uri>;
 
 	constructor(uri: Uri | URL, opts?: PacProxyAgentOptions<Uri>) {
 		super(opts);
@@ -168,6 +174,16 @@ export class PacProxyAgent<Uri extends string> extends Agent {
 		return buf.toString('utf8');
 	}
 
+	addRequest(req: InternalClientRequest, opts: AgentConnectOpts): void {
+		if (this.httpProxyAgent) {
+			// If we connected with an HttpProxyAgent, we need to use it to rewrite the URL and add headers for every request.
+			// We can't just call `this.httpProxyAgent.addRequest(req, opts)` here, because it will create a new socket.
+			this.httpProxyAgent.setRequestProps(req, opts);
+		}
+		// @ts-expect-error `addRequest()` isn't defined in `@types/node`
+		super.addRequest(req, opts);
+	}
+
 	/**
 	 * Called when the node-core HTTP client library is creating a new HTTP request.
 	 */
@@ -236,7 +252,10 @@ export class PacProxyAgent<Uri extends string> extends Agent {
 					const servername = opts.servername || opts.host;
 					socket = tls.connect({
 						...opts,
-						servername: (!servername || net.isIP(servername)) ? undefined : servername,
+						servername:
+							!servername || net.isIP(servername)
+								? undefined
+								: servername,
 					});
 				} else {
 					socket = net.connect(opts);
@@ -260,11 +279,15 @@ export class PacProxyAgent<Uri extends string> extends Agent {
 				if (secureEndpoint) {
 					agent = new HttpsProxyAgent(proxyURL, this.opts);
 				} else {
-					agent = new HttpProxyAgent(proxyURL, this.opts);
+					agent = new HttpProxyAgent(proxyURL, {
+						...this.opts,
+						setRequestPropsOnConnect: true,
+					});
 				}
 			}
 
 			try {
+				this.httpProxyAgent = undefined;
 				if (socket) {
 					// "DIRECT" connection, wait for connection confirmation
 					await once(socket, 'connect');
@@ -279,6 +302,9 @@ export class PacProxyAgent<Uri extends string> extends Agent {
 						);
 					}
 					req.emit('proxy', { proxy, socket: s });
+					if (agent instanceof HttpProxyAgent) {
+						this.httpProxyAgent = agent;
+					}
 					return s;
 				}
 				throw new Error(`Could not determine proxy type for: ${proxy}`);