From 5c6976bb75836f80af067445d08987d6ff145cc8 Mon Sep 17 00:00:00 2001 From: Luigi Pinca Date: Mon, 18 Dec 2023 13:52:24 +0100 Subject: [PATCH 1/2] http: remove misleading warning There are cases where the `'clientError'` event can be emitted multiple times, even if the socket is correctly destroyed. Fixes: https://github.com/nodejs/node/issues/51073 --- lib/_http_server.js | 15 ----- .../test-http-server-multiple-client-error.js | 55 +++++++++++++++++++ .../test-http-socket-error-listeners.js | 4 +- 3 files changed, 56 insertions(+), 18 deletions(-) create mode 100644 test/parallel/test-http-server-multiple-client-error.js diff --git a/lib/_http_server.js b/lib/_http_server.js index 775090b373f696..1222799124cc30 100644 --- a/lib/_http_server.js +++ b/lib/_http_server.js @@ -866,27 +866,12 @@ const requestHeaderFieldsTooLargeResponse = Buffer.from( 'Connection: close\r\n\r\n', 'ascii', ); -function warnUnclosedSocket() { - if (warnUnclosedSocket.emitted) { - return; - } - - warnUnclosedSocket.emitted = true; - process.emitWarning( - 'An error event has already been emitted on the socket. ' + - 'Please use the destroy method on the socket while handling ' + - "a 'clientError' event.", - ); -} - function socketOnError(e) { // Ignore further errors this.removeListener('error', socketOnError); if (this.listenerCount('error', noop) === 0) { this.on('error', noop); - } else { - warnUnclosedSocket(); } if (!this.server.emit('clientError', e, this)) { diff --git a/test/parallel/test-http-server-multiple-client-error.js b/test/parallel/test-http-server-multiple-client-error.js new file mode 100644 index 00000000000000..eaffa305d4ee1f --- /dev/null +++ b/test/parallel/test-http-server-multiple-client-error.js @@ -0,0 +1,55 @@ +'use strict'; +const common = require('../common'); + +// Test that the `'clientError'` event can be emitted multiple times even if the +// socket is correctly destroyed and that no warning is raised. + +const assert = require('assert'); +const http = require('http'); +const net = require('net'); + +process.on('warning', common.mustNotCall()); + +const socketListener = (socket) => { + const firstByte = socket.read(1); + if (firstByte === null) { + socket.once('readable', () => { + socketListener(socket); + }); + return; + } + + socket.unshift(firstByte); + httpServer.emit('connection', socket); +}; + +const netServer = net.createServer(socketListener); +const httpServer = http.createServer(common.mustNotCall()); + +httpServer.once('clientError', common.mustCall((err, socket) => { + assert.strictEqual(err.code, 'HPE_INVALID_METHOD'); + assert.strictEqual(err.rawPacket.toString(), 'Q'); + socket.destroy(); + + httpServer.once('clientError', common.mustCall((err) => { + assert.strictEqual(err.code, 'HPE_INVALID_METHOD'); + assert.strictEqual( + err.rawPacket.toString(), + 'WE http://example.com HTTP/1.1\r\n\r\n' + ); + })); +})); + +netServer.listen(0, () => { + const socket = net.createConnection(netServer.address().port); + + socket.on('connect', common.mustCall(() => { + socket.end('QWE http://example.com HTTP/1.1\r\n\r\n'); + })); + + socket.on('close', () => { + netServer.close(); + }); + + socket.resume(); +}); diff --git a/test/parallel/test-http-socket-error-listeners.js b/test/parallel/test-http-socket-error-listeners.js index f0d220a5d9337a..558bb35882b6ad 100644 --- a/test/parallel/test-http-socket-error-listeners.js +++ b/test/parallel/test-http-socket-error-listeners.js @@ -1,5 +1,3 @@ -// Flags: --no-warnings - 'use strict'; const common = require('../common'); @@ -17,7 +15,7 @@ const net = require('net'); { let i = 0; let socket; - process.on('warning', common.mustCall()); + process.on('warning', common.mustNotCall()); const server = http.createServer(common.mustNotCall()); From 2729b766717136a36d15d66e8145b5959c1296c7 Mon Sep 17 00:00:00 2001 From: Luigi Pinca Date: Tue, 19 Dec 2023 18:35:18 +0100 Subject: [PATCH 2/2] fixup! http: remove misleading warning --- test/parallel/test-http-server-multiple-client-error.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/parallel/test-http-server-multiple-client-error.js b/test/parallel/test-http-server-multiple-client-error.js index eaffa305d4ee1f..4ac41d295e3d3e 100644 --- a/test/parallel/test-http-server-multiple-client-error.js +++ b/test/parallel/test-http-server-multiple-client-error.js @@ -10,7 +10,7 @@ const net = require('net'); process.on('warning', common.mustNotCall()); -const socketListener = (socket) => { +function socketListener(socket) { const firstByte = socket.read(1); if (firstByte === null) { socket.once('readable', () => { @@ -21,7 +21,7 @@ const socketListener = (socket) => { socket.unshift(firstByte); httpServer.emit('connection', socket); -}; +} const netServer = net.createServer(socketListener); const httpServer = http.createServer(common.mustNotCall()); @@ -40,7 +40,7 @@ httpServer.once('clientError', common.mustCall((err, socket) => { })); })); -netServer.listen(0, () => { +netServer.listen(0, common.mustCall(() => { const socket = net.createConnection(netServer.address().port); socket.on('connect', common.mustCall(() => { @@ -52,4 +52,4 @@ netServer.listen(0, () => { }); socket.resume(); -}); +}));