The _stream_wrap module is deprecated

[mvasin]

On node 12.13.0 I see this warning:

(node:3822) [DEP0125] DeprecationWarning: The _stream_wrap module is deprecated.

It can be a low priority, of course, but it's nice to keep track of it.

[pimterry]

Yes, good to keep track of this.

Some context: this is used in https://github.com/httptoolkit/httpolyglot to ensure we can work around certain node issues. _stream_wrap exposes a wrapper class, that wraps a stream so you can use it as a net.Socket directly. E.g. so you can create a TLS connection on top of any duplex stream. When we set up connections, if they're backed by such a wrapper rather than a raw net socket, then the servers avoid making certain (incorrect) assumptions about the raw socket buffers under the hood. We add wrappers for some HTTP/2 cases, to fix some assumptions made there.

I've been talking to the node team about this already, there's further context in nodejs/node#34296 if you're interested. The conclusion is that we shouldn't actually need _stream_wrap here, because every use case should actually be supported and working, so these are fundamentally just workarounds for node bugs, but right now those node bugs still exist, so it's unfortunately hard to remove in the short term. That's true for every currently supported node release, AFAIK, but hopefully there'll be progress there soon! Once those issues disappear, we can remove this (at least, when using new node releases).

If the deprecation is a big problem in the meantime, we could do an inline require instead in httppolyglot, so that it only appears when the first HTTP/2 request appears. I'm inclined not to by default though, just to keep things simple, unless it does start to cause problems.

[mvasin]

That's a brilliant follow-up, thanks Tim! Sure it doesn't bother that much and seems like keeping the warning makes more sense than getting rid of it. I'll close the issue for now because it's not actionable at the moment, let's get back to it in a few years.

[pb-yuri-zaporozhets]

https://github.com/httptoolkit/httpolyglot/blob/master/src/index.ts#L139
https://github.com/nodejs/node/blob/17821703ccf23df62cbef4d6a1d490201118d8d3/lib/internal/http2/core.js#L1197

socket._handle.isStreamBase = false
h2Server.emit('connection', socket)

Can get rid of warning forced to use JSStreamSocket from inside Http2

@pimterry

[pimterry]

Hi @YuraDev. Good idea! I just gave this a quick test though, and using that in the Mockttp tests results in Cannot set property 'isStreamBase' of undefined - at least one of the sockets we're using doesn't have a _handle at this point. We could assign a handle, but I'm not sure what the potential consequences might be.

If you're interested in investigating further though and trying to find a fix for this, I would definitely appreciate that.

You test any changes by:

• Cloning this repo & https://github.com/httptoolkit/httpolyglot/
• Running npm install in both
• Running npm link in httpolyglot and npm link @httptoolkit/httpolyglot in mockttp, so that Mockttp uses your local clone of httpolyglot instead of the published package
• Making some changes in either package
• Running npm run build in httpolyglot to build that
• Running npm run build:src in mockttp to build that
• Running npm run test:node in mockttp to run the quick node tests

If you can find a change that avoids this deprecation and still passes those tests, I would love to hear about it!

[pb-yuri-zaporozhets]

_handle missing Http2Stream

if (socket._handle) {
    socket._handle.isStreamBase = false
}

if (!socket._handle || !socket._handle.isStreamBase)

@pimterry

[pb-yuri-zaporozhets]

Hack

JSStreamSocket

import * as tls from 'tls'
import * as stream from 'stream'

const JSStreamSocket = (new tls.TLSSocket(new stream.PassThrough()))._handle._parentWrap.constructor;

@pimterry

[pimterry]

const JSStreamSocket = (new tls.TLSSocket(new stream.PassThrough()))._handle._parentWrap.constructor;

😆 very clever!

I am a bit cautious about some of these Node.js internal tricks. I don't want to do anything too clever and replace a deprecation warning with a dependency on fields that may disappear and break things completely at any time though. At least _stream_wrap works predictably in Node today, and won't disappear suddenly without a future major version bump.

That 2nd patch does seem to work reliably though - it's easy to write so that it doesn't break if internals change, and after digging through the Node code I'm reasonably confident there's no side effects here. There is however a risk that the internal logic for this will change in future, stopping this working (isStreamBase was only added fairly recently, in Node v12).

We could detect that easily enough (check whether a real stream has an isStreamBase property should work) and then fall back to _stream_wrap in that case only I think.

I'm also going to do some debugging in Node.js itself today, and see if I can find the underlying cause of these HTTP/2 issues, so we don't need the stream wrapper at all in future.

Either way, this looks promising, thanks for the suggestion!

[pimterry]

Now fixed in Mockttp v2.5.0. The patch above is used to avoid the deprecation warning for all Node releases > v12. For v12 and older _stream_wrap is still required (the fix described here doesn't work) but v12 is EOL in April so I'm fine with that.

I've also opened a PR to fix the underlying issue in Node itself in nodejs/node#41185. That's approved by a few Node maintainers now, but not yet merged. Hopefully that'll make it in for Node v18 and potentially get backported as well, which will allow us to remove the patch here entirely, and should help a fair few other HTTP/2 use cases too.