Version

v18.16.0

Platform

Both Windows + Linux, many different devices

Subsystem

net

What steps will reproduce the bug?

Unfortunately, I can't reproduce this myself. It's happening at frequent intervals to many of my users though, crashing Node on their machines when runninghttptoolkit-server (an HTTP debugging proxy) locally.

This issue has just appeared (10s of times a day now) after deploying an update from Node v16.20.1 to v18.16.0.

Internally, this code useshttps://github.com/httptoolkit/mockttp/ which does plenty of interesting things with networking & streams that could be related, including running TLS, HTTP and WebSockets on top of other HTTP/1 connections & HTTP/2 streams for traffic proxying purposes. The relevant code for that is here:https://github.com/httptoolkit/mockttp/blob/main/src/server/http-combo-server.ts.

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

From the direct reports I've had, it seems this happens intermittently after handling a few minutes of HTTP traffic. The client is Chrome is every case I'm aware of, configured to proxy traffic via Node.

There's no obvious trigger on the server side - each user seems to be doing entirely different things there - but it's likely that a good amount of live web traffic is travelling through this stream so some kind of race condition seems very plausible.

What is the expected behavior? Why is that the expected behavior?

It shouldn't crash! 😄

What do you see instead?

It crashes. The full stack trace I have is:

TypeError: Cannot read properties of null (reading 'finishShutdown')  File "node:internal/js_stream_socket", line 160, col 12, in JSStreamSocket.finishShutdown  File "node:internal/js_stream_socket", line 147, col 14, in null.<anonymous>  File "node:internal/process/task_queues", line 81, col 21, in process.processTicksAndRejections

Additional information

Obviously this isn't the most helpful bug report - there's no repro, and I have no easy way to quickly test possible solutions.

That said, I'm hoping somebody might be aware of what changes could be related and triggering this that I can investigate, or might know more about possible race conditions in the way thatfinishShutdown is used that can be tied up to avoid issues like this.

From a quick skim of the JSStreamSocket code, this looks suspicious to me:

• Error ishere which meansfinishShutdown is being called withnull as the first argument.
• That was called indoShutdownhere which meansthis._handle was null whendoShutdown was called.
• We can't tell in this setup what triggered that, butfinishWrite would do this if a shutdown was deferred to wait for a write to complete (there's a TODO indoShutdown from@addaleax specifically pointing out that this is a bit messy).
• IndoClose,this._handle will be nullhere immediately before a call tofinishWrite.

So, AFAICT: if something callsdoShutdown with a pending write (deferring the shutdown) and then callsdoClose before the shutdown completes,doClose will clear_handle and then callfinishWrite, and that will then calldoShutdown to finish the pending shutdown, which will crash.

Does that make sense? Is there a reason that can't happen? There may be other cases but from the outside that seems like the clearest series that would lead to a crash here.

If that's the cause, some possible fixes:

• It would seem easy to just passhandle intodoShutdown (everywhere we call it here, we know we have the handle still in scope) but I don't know if that has larger implications - it looks likedoShutdown is part of the interface used elsewhere so we can't just change it.

• Maybe we can just return out ofdoShutdown if thethis._handle is alreadynull - that means something else is already shutting everything down, so we shouldn't get involved (in this case,doClose callsfinishShutdown for us straight after callingfinishWrite so there is indeed no need for this).

• MaybedoClose should check for this - if you callfinishWrite with a shutdown pending after destroying the stream, you're in trouble.doClose callsfinishShutdown itself anyway, so it would seem reasonable for it to clear pending shutdowns because it's going to handle the shutdown itself.

The only change in this file between these versions is9ccf8b2 from@lpinca, which did change the logic directly around the failing code! Plausibly switching from setImmediate to process.nextTick meant that the IO events triggering this are now firing in a different order, causing the above race? Seems we should protect against the race regardless, but it's a plausible trigger imo.