Skip to content

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

Closed
@pimterry

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.

Metadata

Assignees

No one assigned

    Labels

    httpIssues or PRs related to the http subsystem.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions