HTTP server can report clientError misuse warning even when socket is destroyed correctly

[pimterry]

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.