From 7c06eab1dce3b8f05546b9dc13d3969a12d0ba2e Mon Sep 17 00:00:00 2001 From: Luigi Pinca Date: Wed, 12 Oct 2022 14:16:56 +0200 Subject: [PATCH] Revert "http: do not leak error listeners" This reverts commit 736a7d8d60acdcf3274ae160fadefbbbbe4e4311. The patch attempted to fix an issue, signaled by a warning, caused by an invalid API usage. However it introduced a behavior change that is breaking userland modules. It is a user error, so restore the original behavior and have the user investigate and fix the issue in their code. Refs: https://github.com/nodejs/node/pull/43587#issuecomment-1272092166 Refs: https://github.com/nodejs/node/pull/43587#issuecomment-1272094883 PR-URL: https://github.com/nodejs/node/pull/44921 Reviewed-By: Matteo Collina Reviewed-By: Robert Nagy Reviewed-By: Colin Ihrig Reviewed-By: Beth Griggs Reviewed-By: Tierney Cyren --- lib/_http_server.js | 5 +-- test/parallel/test-http-socket-listeners.js | 44 --------------------- 2 files changed, 1 insertion(+), 48 deletions(-) delete mode 100644 test/parallel/test-http-socket-listeners.js diff --git a/lib/_http_server.js b/lib/_http_server.js index 12d4c5b38e9e49..f882ee7d2a6cb1 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -809,10 +809,7 @@ const requestHeaderFieldsTooLargeResponse = Buffer.from( function socketOnError(e) { // Ignore further errors this.removeListener('error', socketOnError); - - if (this.listenerCount('error') === 0) { - this.on('error', noop); - } + this.on('error', noop); if (!this.server.emit('clientError', e, this)) { // Caution must be taken to avoid corrupting the remote peer. diff --git a/test/parallel/test-http-socket-listeners.js b/test/parallel/test-http-socket-listeners.js deleted file mode 100644 index 2513bc1a90c9f6..00000000000000 --- a/test/parallel/test-http-socket-listeners.js +++ /dev/null @@ -1,44 +0,0 @@ -'use strict'; - -const common = require('../common'); -const assert = require('assert'); -const http = require('http'); -const net = require('net'); - -// This test sends an invalid character to a HTTP server and purposely -// does not handle clientError (even if it sets an event handler). -// -// The idea is to let the server emit multiple errors on the socket, -// mostly due to parsing error, and make sure they don't result -// in leaking event listeners. - -let i = 0; -let socket; - -process.on('warning', common.mustNotCall()); - -const server = http.createServer(common.mustNotCall()); - -server.on('clientError', common.mustCallAtLeast((err) => { - assert.strictEqual(err.code, 'HPE_INVALID_METHOD'); - assert.strictEqual(err.rawPacket.toString(), '*'); - - if (i === 20) { - socket.end(); - } else { - socket.write('*'); - i++; - } -}, 1)); - -server.listen(0, () => { - socket = net.createConnection({ port: server.address().port }); - - socket.on('connect', () => { - socket.write('*'); - }); - - socket.on('close', () => { - server.close(); - }); -});