HTTP/2 server does not accept injected 'connection' events for sockets with peeked data

[pimterry]

• Version: probably all (at least 8.4.0, 10.21.0 & 12.18.3)
• Platform: all
• Subsystem: HTTP/2

What steps will reproduce the bug?

Start a server like so:

const net = require('net');
const http = require('http');
const http2 = require('http2');
const Wrapper = require('_stream_wrap');

const rawConnListener = (socket) => {
    const data = socket.read(3);

    if (!data) { // Repeat until data is available
        socket.once('readable', () => rawConnListener(socket));
        return;
    }

    // Put the data back, so the real server can handle it:
    socket.unshift(data);

    if (data.toString('ascii') === 'PRI') { // Very dumb preface check
        console.log('emitting h2');
        h2Server.emit('connection', socket);
    } else {
        console.log('emitting h1');
        h1Server.emit('connection', socket);
    }
}

const rawServer = net.createServer(rawConnListener);

const h1Server = http.createServer(() => {
    console.log('Got h1 request');
});

const h2Server = http2.createServer(async () => {
    console.log('Got h2 request');
});

rawServer.listen(8080);

Make a direct HTTP/2 request to it:

curl --http2-prior-knowledge http://localhost:8080

Curl will fail ('Error in the HTTP2 framing layer'), and the server will print 'emitting h2' but never 'Got h2 request'.

How often does it reproduce? Is there a required condition?

Happens every time.

What is the expected behavior?

The server should accept emitted 'connection' events for any Duplex stream, including existing sockets, as discussed in #34296 (comment):

HTTP/2 should support h2Server.emit('connection', socket), just like HTTP/1 does. I think the main problem here is the unshifted data? That’s something that we can (and should) take care of, yes.

This does work correctly for HTTP/1 servers, and HTTP/1 requests using the code above, and is documented as a supported use case. This isn't currently documented as a supported API for HTTP/2, but there's a PR open for that: #34531.

What do you see instead?

HTTP/2 connections are never successfully established for sockets emitted like this.

[leandro-manifesto]

The documentation was updated but it still doesn't work.

[pimterry]

Just added a note in #34296 (comment) on this. To reiterate here: this is still an issue as of Node v16.8.0 (EDIT: still an issue today in v17.2.0).

I've also just found another use case: the Docker Engine BuildKit API.

This API does some fascinating & funky things to create an HTTP/2 connection. Because of that, connecting to this API always requires manually emitting a connection event to an HTTP/2 server from a raw socket after an HTTP/1.1 upgrade. Using the normal Node HTTP APIs, that means some data (the HTTP/2 preamble) has often already been read into the head buffer parameter, and so must be unshifted back into the socket before you can create the HTTP/2 connection.

This fails just like the example above, making the BuildKit API unusable in Node right now unless you manually use _stream_wrap. Fortunately it's not a properly documented or widely used API, since it's only just become stable, but it's likely to get more popular in future and it'd be very useful to be able to work with this API from Node.