From 490309294458c82cb342774cebdfe3acec08729d Mon Sep 17 00:00:00 2001 From: Parth Verma Date: Wed, 10 Jul 2024 22:56:07 -0700 Subject: [PATCH] Added socket caching to Auto Agent to handle the case when http2 request and http1 requests are made to the same host+port --- lib/autohttp/agent.js | 108 +++++++++++++++++++----------- lib/autohttp/headerValidations.js | 1 - lib/http2/agent.js | 11 ++- lib/http2/request.js | 2 +- lib/redirect.js | 2 +- request.js | 1 - 6 files changed, 78 insertions(+), 47 deletions(-) diff --git a/lib/autohttp/agent.js b/lib/autohttp/agent.js index bd72297f1..dd0d101c7 100644 --- a/lib/autohttp/agent.js +++ b/lib/autohttp/agent.js @@ -61,59 +61,75 @@ class AutoHttp2Agent extends EventEmitter { cb, socketCb ) { - const options = Object.assign({}, reqOptions, this.options) + let options = Object.assign({}, reqOptions, this.options) + options = Object.assign(options, { + port: Number(options.port || this.defaultPort), + host: options.hostname || options.host || 'localhost' + }) + + // check if ALPN is cached const name = this.getName(options) + const [protocol, cachedSocket] = this.ALPNCache.get(name) || [] - const uri = httpOptionsToUri(options) - const port = Number(options.port || this.defaultPort) + if (!protocol || !cachedSocket || cachedSocket.closed || cachedSocket.destroyed) { + // No cache exists or the initial socket used to establish the connection has been closed. Perform ALPN again. + this.ALPNCache.delete(name) + this.createNewSocketConnection(req, options, cb, socketCb) + return + } - // check if there is ALPN cached - const protocol = this.ALPNCache.get(name) + // No need to pass the cachedSocket since the respective protocol's agents will reuse the socket that was initially + // passed during ALPN Negotiation if (protocol === 'h2') { - const newOptions = Object.assign({}, options, { - port, - path: options.socketPath, - host: options.hostname || options.host || 'localhost' + const http2Options = Object.assign({}, options, { + path: options.socketPath }) let connection try { - connection = this.http2Agent.createConnection(req, uri, newOptions) + const uri = httpOptionsToUri(options) + connection = this.http2Agent.createConnection(req, uri, http2Options) } catch (e) { cb(e) connection && connection.socket && socketCb(connection.socket) return } + cb(null, 'http2', connection) socketCb(connection.socket) + return } + if (protocol === 'http/1.1' || protocol === 'http/1.0') { - const requestOptions = Object.assign({}, options, { - agent: this.httpsAgent, - host: options.hostname || options.host || 'localhost' + const http1RequestOptions = Object.assign({}, options, { + agent: this.httpsAgent }) + let request try { - request = https.request(requestOptions) + request = https.request(http1RequestOptions) } catch (e) { cb(e) return } + request.on('socket', (socket) => socketCb(socket)) cb(null, 'http1', request) - return } + } + + createNewSocketConnection (req, options, cb, socketCb) { + const uri = httpOptionsToUri(options) + const name = this.getName(options) - const newOptions = Object.assign({}, options, { - port, + const tlsSocketOptions = Object.assign({}, options, { path: options.socketPath, ALPNProtocols: ['h2', 'http/1.1', 'http/1.0'], - servername: options.servername || calculateServerName(options), - host: options.hostname || options.host || 'localhost' + servername: options.servername || calculateServerName(options) }) - const socket = tls.connect(newOptions) + const socket = tls.connect(tlsSocketOptions) socketCb(socket) const socketConnnectionErrorHandler = (e) => { @@ -132,43 +148,54 @@ class AutoHttp2Agent extends EventEmitter { return } - this.ALPNCache.set(name, protocol) + if (protocol !== 'h2' && protocol !== 'http/1.1') { + cb(new Error('Unknown protocol' + protocol)) + return + } + + // Update the cache + this.ALPNCache.set(name, [protocol, socket]) + + socket.on('close', () => { + // Clean the cache when the socket closes + this.ALPNCache.delete(name) + }) if (protocol === 'h2') { - const newOptions = Object.assign({}, options, { - port, - path: options.socketPath, - host: options.hostname || options.host || 'localhost' + const http2Options = Object.assign({}, options, { + path: options.socketPath }) try { const connection = this.http2Agent.createConnection( req, uri, - newOptions, + http2Options, socket ) cb(null, 'http2', connection) } catch (e) { cb(e) } - } else if (protocol === 'http/1.1') { - // Protocol is http1, using the built in - // We need to release all free sockets so that new connection is created using the overriden createconnection forcing the agent to reuse the socket used for alpn + } + if (protocol === 'http/1.1' || protocol === 'http/1.0') { + // Protocol is http1, using the built in agent + // We need to release all free sockets so that new connection is created using the overridden createconnection + // forcing the agent to reuse the socket used for alpn - // This reassignment works, since all code so far is sync, and happens in the same tick, hence there will be no race conditions + // This reassignment works, since all code so far is sync, and happens in the same tick, hence there will be no + // race conditions const oldCreateConnection = this.httpsAgent.createConnection this.httpsAgent.createConnection = () => { return socket } - const requestOptions = Object.assign({}, options, { - agent: this.httpsAgent, - host: options.hostname || options.host || 'localhost' + const http1RequestOptions = Object.assign({}, options, { + agent: this.httpsAgent }) let request try { - request = https.request(requestOptions) + request = https.request(http1RequestOptions) } catch (e) { cb(e) return @@ -176,16 +203,14 @@ class AutoHttp2Agent extends EventEmitter { this.httpsAgent.createConnection = oldCreateConnection } cb(null, 'http1', request) - } else { - cb(new Error('Unknown protocol' + protocol)) } }) } - /* - * This function has been borrowed from Node.js HTTPS Agent implementation - * Ref: v20.15.0 https://github.com/nodejs/node/blob/6bf148e12b00a3ec596f4c123ec35445a48ab209/lib/https.js - */ + /* + * This function has been borrowed from Node.js HTTPS Agent implementation + * Ref: v20.15.0 https://github.com/nodejs/node/blob/6bf148e12b00a3ec596f4c123ec35445a48ab209/lib/https.js + */ getName (options) { let name = options.host || 'localhost' @@ -200,6 +225,9 @@ class AutoHttp2Agent extends EventEmitter { name += ':' if (options.ca) { name += options.ca } + name += ':' + if (options.extraCA) { name += options.extraCA } + name += ':' if (options.cert) { name += options.cert } diff --git a/lib/autohttp/headerValidations.js b/lib/autohttp/headerValidations.js index 67060d12e..4d40cb54a 100644 --- a/lib/autohttp/headerValidations.js +++ b/lib/autohttp/headerValidations.js @@ -22,7 +22,6 @@ function checkIsHttpToken (token) { return RegExp(tokenRegExp).exec(token) !== null } - module.exports = { assertValidPseudoHeader, checkIsHttpToken diff --git a/lib/http2/agent.js b/lib/http2/agent.js index d35b212c3..9fe5c1f0d 100644 --- a/lib/http2/agent.js +++ b/lib/http2/agent.js @@ -15,9 +15,11 @@ class Http2Agent extends EventEmitter { if (!connection || connection.destroyed || connection.closed) { const connectionOptions = Object.assign({}, options, - { createConnection: undefined, port: _options.port || 443, settings: { - enablePush: false - } }) + { createConnection: undefined, + port: _options.port || 443, + settings: { + enablePush: false + } }) // check if a socket is supplied if (socket) { @@ -89,6 +91,9 @@ class Http2Agent extends EventEmitter { name += ':' if (options.ca) { name += options.ca } + name += ':' + if (options.extraCA) { name += options.extraCA } + name += ':' if (options.cert) { name += options.cert } diff --git a/lib/http2/request.js b/lib/http2/request.js index 4af973f2a..9440548a5 100644 --- a/lib/http2/request.js +++ b/lib/http2/request.js @@ -165,7 +165,7 @@ class ResponseProxy extends EventEmitter { this.response = response this.on = this.on.bind(this) this.registerRequestListeners() - this.socket = this.reqStream.session.socket; + this.socket = this.reqStream.session.socket } registerRequestListeners () { diff --git a/lib/redirect.js b/lib/redirect.js index 2e5662791..7fd996561 100644 --- a/lib/redirect.js +++ b/lib/redirect.js @@ -210,7 +210,7 @@ Redirect.prototype.onResponse = function (response) { request.emit('redirect') - request.response.once('end', () =>{ + request.response.once('end', () => { request.init(options) }) diff --git a/request.js b/request.js index 4d657ff3e..b10178dd9 100644 --- a/request.js +++ b/request.js @@ -1278,7 +1278,6 @@ Request.prototype.onRequestResponse = function (response) { } debug('response end', self.uri.href, response.statusCode, response.headers) - }) if (self._aborted) {