HTTP server can report clientError misuse warning even when socket is destroyed correctly #51073
Description
Version
v20.10.0
Platform
Linux 6.2.0-37-generic 38-Ubuntu SMP PREEMPT_DYNAMIC Mon Oct 30 21:04:52 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux
Subsystem
http
What steps will reproduce the bug?
const net = require('net');
const http = require('http');
async function sendRawRequest(server, requestContent) {
const client = new net.Socket();
await new Promise((resolve) => client.connect(server.address().port, '127.0.0.1', resolve));
const socketPromise = new Promise((resolve, reject) => {
client.on('end', resolve);
client.on('error', reject);
});
client.write(requestContent);
client.end();
return socketPromise;
}
const socketListener = (socket) => {
const firstByte = socket.read(1);
if (firstByte === null) {
socket.once('readable', () => socketListener(socket));
return;
}
console.log('First byte is', firstByte.toString());
socket.unshift(firstByte);
httpServer.emit('connection', socket);
};
const netServer = net.createServer(socketListener);
const httpServer = http.createServer((req, res) => {
throw new Error('never reached');
});
httpServer.on('clientError', (err, socket) => {
console.log('Got client error:', err.message);
console.log('Data:', err.rawPacket.toString('utf8'));
socket.destroy();
})
netServer.listen(0, async () => {
await sendRawRequest(netServer, 'QWE http://example.com HTTP/1.1\r\n\r\n').catch(() => {});
netServer.close();
});
This prints:
First byte is Q
Got client error: Parse Error: Invalid method encountered
Data: Q
Got client error: Parse Error: Invalid method encountered
Data: WE http://example.com HTTP/1.1
(node:1280016) Warning: An error event has already been emitted on the socket. Please use the destroy method on the socket while handling a 'clientError' event.
(Use `node --trace-warnings ...` to show where the warning was created)
How often does it reproduce? Is there a required condition?
Always happens.
Requires the unshift peeking step shown in this example.
Affects all Node versions since this change by @ShogunPanda.
What is the expected behavior? Why is that the expected behavior?
The warning should not be printed - the socket was destroyed immediately on the first error.
What do you see instead?
A warning is always printed, but it's incorrect - destroy was indeed called immediately.
Additional information
This is a slightly unusual setup: the socket data has been peeked and unshifted before the HTTP parser throws its error. That said, this is explicitly supported and tested (see test-http2-autoselect-protocol.js for example). This is used in production by quite a few people (at the least, all users of https://www.npmjs.com/package/httpolyglot and https://www.npmjs.com/package/@httptoolkit/httpolyglot) because it's the only way to sniff packet protocols before handling them.
You could argue that the second error event here is the real problem - Node is firing the second error when it doesn't expect to, and we could just suppress that. Disabling this would resolve the warning, but it would cause other issues: currently you need to collect the error.rawPacket
data from both errors to reconstruct the original failing request (you can see the Q and WE are split in the console output shown). That's a separate bug itself really, but worth mentioning here - fixing this without fixing that would cause me real problems!
One working solution would be to nextTick/setImmediate the clientError event, and thereby collect the full packet data from any pending data into rawPacket, before throwing the error. Is that worth investigating?
I'm happy to open a PR/PRs to resolve both issues if that seems like a reasonable solution, or if anybody has a clear alternative.