From c3bfa18e5d069ef5c1a01074d7f44652556530bf Mon Sep 17 00:00:00 2001 From: Parth Verma <75295512+parthverma1@users.noreply.github.com> Date: Fri, 26 Jul 2024 00:16:52 -0700 Subject: [PATCH] Added exception handlers when removing listeners from http2 sockets (#95) --- lib/http2/request.js | 58 +++++++++++++++++++++++++++-------------- request.js | 22 +++++++++++++--- tests/test-ssl-http2.js | 42 +++++++++++++++++++++++++++-- 3 files changed, 97 insertions(+), 25 deletions(-) diff --git a/lib/http2/request.js b/lib/http2/request.js index ac6008463..564f99177 100644 --- a/lib/http2/request.js +++ b/lib/http2/request.js @@ -38,6 +38,11 @@ class Http2Request extends EventEmitter { constructor (options) { super() this.onError = this.onError.bind(this) + this.onDrain = this.onDrain.bind(this) + this.onClose = this.onClose.bind(this) + this.onResponse = this.onResponse.bind(this) + this.onEnd = this.onEnd.bind(this) + this.registerListeners = this.registerListeners.bind(this) this._flushHeaders = this._flushHeaders.bind(this) this[kHeadersFlushed] = false @@ -92,32 +97,45 @@ class Http2Request extends EventEmitter { } registerListeners () { - this.stream.on('drain', () => this.emit('drain', arguments)) - this.stream.on('error', (e) => this.emit('error', e)) + this.stream.on('drain', this.onDrain) + this.stream.on('error', this.onError) + this.stream.on('close', this.onClose) + this.stream.on('response', this.onResponse) + this.stream.on('end', this.onEnd) + } - this.stream.on('close', (...args) => { - if (this.stream.rstCode) { - // Emit error message in case of abnormal stream closure - this.onError(new Error(`HTTP/2 Stream closed with error code ${rstErrorCodesMap[this.stream.rstCode]}`)) - } + onDrain (...args) { + this.emit('drain', ...args) + } - this.emit('close', args) - this._client.off('error', this.onError) - this.stream.removeAllListeners() - this.removeAllListeners() - }) + onError (e) { + this.emit('error', e) + } - this.stream.on('response', (response) => { - this.emit('response', new ResponseProxy(response, this.stream)) - }) + onResponse (response) { + this.emit('response', new ResponseProxy(response, this.stream)) + } - this.stream.on('end', () => { - this.emit('end') - }) + onEnd () { + this.emit('end') } - onError (e) { - this.emit('error', e) + onClose (...args) { + if (this.stream.rstCode) { + // Emit error message in case of abnormal stream closure + this.onError(new Error(`HTTP/2 Stream closed with error code ${rstErrorCodesMap[this.stream.rstCode]}`)) + } + + this.emit('close', ...args) + + this._client.off('error', this.onError) + this.stream.off('drain', this.onDrain) + this.stream.off('error', this.onError) + this.stream.off('response', this.onResponse) + this.stream.off('end', this.onEnd) + this.stream.off('close', this.onClose) + + this.removeAllListeners() } setDefaultEncoding (encoding) { diff --git a/request.js b/request.js index 1d574ef62..f5e4ca3a8 100644 --- a/request.js +++ b/request.js @@ -1139,8 +1139,16 @@ Request.prototype.start = function () { // clean up timing event listeners if needed on error self.req.once('error', function () { - socket.removeListener('lookup', onLookupTiming) - socket.removeListener('connect', onConnectTiming) + // Swallow ERR_HTTP2_SOCKET_UNBOUND error when removing listeners in case of error. + // This needs to be done since http2 ClientSession disassociates the underlying socket from the session before emitting the error event + try { + socket.removeListener('lookup', onLookupTiming) + socket.removeListener('connect', onConnectTiming) + } catch (err) { + if (err.code !== 'ERR_HTTP2_SOCKET_UNBOUND') { + throw err + } + } }) } } @@ -1176,7 +1184,15 @@ Request.prototype.start = function () { socket.on('connect', onReqSockConnect) self.req.on('error', function (err) { // eslint-disable-line handle-callback-err - socket.removeListener('connect', onReqSockConnect) + // Swallow ERR_HTTP2_SOCKET_UNBOUND error when removing listeners in case of error. + // This needs to be done since http2 ClientSession disassociates the underlying socket from the session before emitting the error event + try { + socket.removeListener('connect', onReqSockConnect) + } catch (err) { + if (err.code !== 'ERR_HTTP2_SOCKET_UNBOUND') { + throw err + } + } }) // Set a timeout in memory - this block will throw if the server takes more diff --git a/tests/test-ssl-http2.js b/tests/test-ssl-http2.js index b3ceb0a6e..b1d35365e 100644 --- a/tests/test-ssl-http2.js +++ b/tests/test-ssl-http2.js @@ -26,7 +26,15 @@ var http2SecureServer = server.createHttp2Server({ rejectUnauthorized: true }) +var httpsServer = server.createSSLServer({ + key: path.resolve(__dirname, 'ssl/ca/localhost.key'), + cert: path.resolve(__dirname, 'ssl/ca/localhost.crt'), + ca: caPath, + rejectUnauthorized: true +}) + destroyable(http2SecureServer) +destroyable(httpsServer) tape('setup', function (t) { http2SecureServer.on('/', function (req, res) { @@ -39,8 +47,20 @@ tape('setup', function (t) { } }) + httpsServer.on('/', function (req, res) { + if (req.connection.authorized) { + res.writeHead(200, { 'Content-Type': 'text/plain' }) + res.end('authorized') + } else { + res.writeHead(401, { 'Content-Type': 'text/plain' }) + res.end('unauthorized') + } + }) + http2SecureServer.listen(0, function () { - t.end() + httpsServer.listen(0, function () { + t.end() + }) }) }) @@ -143,8 +163,26 @@ tape('ca + extraCA', function (t) { }) }) +tape('http2 -> https', function (t) { + request({ + url: httpsServer.url, + ca: ca, + key: clientKey, + cert: clientCert, + protocolVersion: 'http2' + }, function (err, res, body) { + t.notEqual(err, null) + t.equal(err.code, 'ERR_HTTP2_ERROR') + t.equal(err.errno, -505) + + t.end() + }) +}) + tape('cleanup', function (t) { http2SecureServer.destroy(function () { - t.end() + httpsServer.destroy(function () { + t.end() + }) }) })