Node: Undeprecate _stream_wrap

[pimterry]

Is your feature request related to a problem? Please describe.

_stream_wrap was recently deprecated because it "doesn't have a reasonable use outside of node".

I think I have two reasonable use cases, and I'd really like this to be undeprecated. As far as I can tell it's going to exist internally anyway, since the underlying JSStreamSocket implementation is actively used by TLSSocket, it works perfectly, and it already supports my use cases, so this is purely a question of whether it should be accessible externally.

My use cases:

1. I'm trying to sniff packets to detect HTTP/2 before the http2 server takes over, as described in #17132 (in my case: because I want to support HTTP/1, HTTP/2, HTTPS/1 and HTTPS/2 all on the same port). As described in that issue that's not normally possible, understandably, because sockets are tightly integrated with the stream internals in a way that disallows these kinds of interactions, and changing that has performance implications for the common case. However, this is possible if you provide some psuedo-socket, backed by a separate stream, such as _stream_wrap.

   There are other users with this use case, including the original author of that issue, and every user of httpolyglot, which does this for HTTP/1 (but which can't HTTP/2 using the native http2 module because of this issue). Issues like #20999 exist because this isn't available, so people have to implement it externally for themselves.

2. I'm trying to tunnel HTTP connections through a stream (in this case, an Http2Stream). I'm building a mitm proxy, which accepts h2 CONNECT requests and then locally handles them. That means after the CONNECT I get an h2 stream, which I need to treat as a fresh net.Socket connection. _stream_wrap does this perfectly.

   This is less common, afaik, but it's an example of wanting the networking APIs to be generally more composeable, so that socket-based code can fully interoperate with streams or stream transformations.

Describe the solution you'd like

Make _stream_wrap a public API again, removing the deprecations.

Maybe even remove the underscore too, and make it a 1st class API with docs etc?

Describe alternatives you've considered

The alternative I can see is to reimplement _stream_wrap from scratch externally.

That's not really straightforward, as implementation here requires native code (https://github.com/nodejs/node/blob/master/src/js_stream.cc), which I'd also need to duplicate, and my pipeline to usefully build & distribute all this ends up being pretty complicated, especially to get it working across node versions.

It would also be far less reliable (I'd much rather use the real & well-maintained version of this, especially since it touches lots of node internals), and it's annoying to have to do all this when a fully working implementation is shipping within node anyway.

[Trott]

@nodejs/streams

[mcollina]

This is really outside of the streams domain.

[Trott]

This is really outside of the streams domain.

OK, thanks. In that case, I guess maybe I'll try pinging the people who authored/approved the deprecation in the first place.

cc @sam-github @BridgeAR @mcollina @jasnell @addaleax @tniessen

[pimterry]

Any thoughts on this, pinged people?

Right now I'm still shipping new code dependent on this API, because there's no easy alternative. It'd be really useful to get an idea of whether there's hope that it'll be undeprecated, or whether I seriously need to start looking at the practicality of extracting the existing built-in module as a native node addon.

[mcollina]

I think it's possible to undeprecate it and make it a public module. However this is significant work: from API design, hardening against user input, docs and tests.

@pimterry Would you like to work on this?

[pimterry]

Yes, I can do that.

I'm very happy to write docs & tests for the existing implementation as I understand it. I can start prepping an initial PR for that straight away, if there's appetite for it.

Is there specific hardening & API design work you have in mind? Happy to work on that if somebody can point me in the direction of outstanding issues there.

[jasnell]

No particular opinions on this. If it's useful to userland then ok by me

[mcollina]

@addaleax wdyt?

[addaleax]

I don’t see any problem with this – I’d probably put it onto the net module instead of having it be its own public module.

That being said: I’m not sure whether this really helps covering the listed use cases – at least HTTP/2, HTTP/1 and TLS should all accept any kind of Duplex stream as the underlying connection, whether it’s wrapped into a net.Socket or not, because they use _stream_wrap internally to achieve that. Maybe I’m missing something here, though.

[pimterry]

I’d probably put it onto the net module instead of having it be its own public module.

Absolutely, makes sense to me.

I’m not sure whether this really helps covering the listed use cases

For the CONNECT case, you're actually completely right @addaleax, I'm wrong - the stream wrapper isn't required with node's built-in servers. In my case at the time I was using node-spdy for HTTP/2 with Express compatibility. That builds on net.Server directly, and does make strong assumptions that connections are sockets, and does need a wrapper. I suspect that's true for other similar userspace networking libraries.

For the socket-peeking case, this code requires the stream wrapper:

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', new Wrapper(socket)); // <-- here
    } 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);

This creates a net.Server, which peeks at the incoming data, and then directs the connection to a different server depending on the first byte. This is effectively what httpolyglot does, to support HTTP 1 & 2, in plain text & HTTPS, all on one port, and similar to the use case described by another user in #17132.

Testing this with the wrapper:

➜ curl http://localhost:8080 
# Server prints 'emitting h1', 'Got h1 request'

➜ curl --http2-prior-knowledge http://localhost:8080
# Server prints 'emitting h2', 'Got h2 request'

Without the wrapper:

➜ curl http://localhost:8080 
# Server prints 'emitting h1', 'Got h1 request'

➜ curl --http2-prior-knowledge http://localhost:8080
curl: (16) Error in the HTTP2 framing layer # <- curl fails
# Server prints 'emitting h2', never prints anything else

This case is not handled by the support in the built-in libraries because the stream is actually a socket, so the servers never wrap it. We need the wrapper here to force the server to treat it as a plain stream, because the user code wants to use it as a stream too.

[mcollina]

@pimterry is that the only use case? If so it might be better to just expose that somehow. I think it'd be a pretty nice feature to have.

[addaleax]

Okay, two things:

• node-spdy is doing things that Node.js cannot reasonably be considered supportable by us, and, from my experience, breaks quite frequently after Node.js updates.
• 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.

[addaleax]

i.e.: I think this issue might be an X-Y problem, and all that might be needed here are bug fixes in order to account for the above use case.

[pimterry]

node-spdy is doing things that Node.js cannot reasonably be considered supportable by us, and, from my experience, breaks quite frequently after Node.js updates.

I'm not using node-spdy (it is indeed quite broken in places right now), so that fine by me.

That said, I can imagine cases of other libraries that expect sockets, where as a user it'd be useful to be able to provide a fake socket from a stream instead, or where the library itself wants to support both easily.

For example, if you wanted to implement any other application protocol server on top of TCP in userspace, with similar behaviour to the built-in http/http2/tls servers, then that requires this stream wrapper, for the exact same reasons that the built-in implementations use it. It seems a shame to keep a working implementation of such a feature internal-only.

I'm personally not doing that though, so happy to ignore that case until somebody who specifically needs it appears, if that's preferable.

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.

The conclusion of https://github.com/nodejs/node/issues/17132 seemed to suggest that this is _not_ something the core implementation would support directly. My impression is that emitting streams manually and similar tricks works, but isn't officially encouraged, to focus the servers on keeping common socket case simple & fast instead.

If that can be fully supported though then that's great! That'd be extremely useful from my pov. I do a lot of that, and fixing this issue specifically would indeed solve my problem.

[addaleax]

My impression is that emitting streams manually and similar tricks works, but isn't officially encouraged, to focus the servers on keeping common socket case simple & fast instead.

I do consider it supported (and it’s tested). Maybe the documentation could be a bit clearer on this as well :+1:

[pimterry]

I do consider it supported (and it’s tested). Maybe the documentation could be a bit clearer on this as well :+1:

Ok, I've opened a PR to improve this somewhat: https://github.com/nodejs/node/pull/34531

And a separate bug for the issue with emitting sockets on an HTTP/2 server: https://github.com/nodejs/node/issues/34532

Independently of those, I think there are still some arguments for supporting _stream_wrap, so I'll leave this issue open. I now don't have a specific case where I personally need it though other than working around bugs (both the new #34532 and existing #29902), so I'm happy for this to be closed if there's consensus that it isn't worthwhile.