Fix JS Stream Socket finishShutdown crash

[pimterry]

This fixes #48519
EDIT: and fixes #46094

This is independent of my other related fix #49327 but in most relevant scenarios you'll want both together. Each of these PRs has a test that catastrophically fails right now, but works correctly with the corresponding change included, but you'll likely want both since in production I find that either fix by itself ends up resulting in the other error crashing my application instead a little later on (i.e. these two separate issues are currently racing to crash my code).

There's a detailed breakdown of the crash fixed in this PR here. To summarize:

• A JS Stream Socket (JSS) is created, wrapping an HTTP/2 stream (anything might work) and then used by a TLS stream (i.e. TLS runs on top of the JSS) with allowHalfOpen = false set (this is common for TLS, AFAICT).
• The underlying H2 stream closes
• This fires a 'close' event, which results in the JSS calling doClose, and also ends the TLS stream (which in turn closes the writable side, due to allowHalfOpen = false) which calls doShutdown on the JSS. Effectively both sides try to shut down the JSS at the same time.
• doClose runs first, calls this.stream.destroy() which sets this._handle to null, and schedules a setImmediate to cancel any pending writes or shutdowns.
• doShutdown then runs, picks up this._handle (now null), and then schedules a finishShutdown that will call this._handle.finishShutdown (=uncatchable NPE)
• Before 9ccf8b2 (released in Node 18+) the doClose callback ran first, cleared kCurrentShutdownRequest, and so the null pointer was never actually read, but effectively just by luck.
• After that change, the finishShutdown from doShutdown runs first, and so does use its null pointer every time, crashing everything.

Rather than just reverting to setImmediate or using process.nextTick in doClose too, I've fixed this properly: now, if there is any race with doClose, the other methods recognize it, store the kCurrent{Shutdown,Write}Request param to allow doClose to do its clean cancellation a moment later, and then they avoid reading or using the null this._handle value entirely.

That above example explains the doShutdown race. A similar race applies to doWrite/finishWrite too, which I can reliably reproduce in my application after fixing just the shutdown case, although I don't have a reliable test to repro this (#46094, #35695, #27258 and microsoft/vscode#188676 all appear to be examples of the same issue). With the equivalent fix included for doWrite here too, I can no longer reproduce that error either.

With the fixes for both methods here plus #49327, I can proxy significant chunks of real HTTP/2 browser traffic through my app, including all the weird edge cases and errors of the web for at least an hour. Previously this crashed comfortably within a minute on Node 18+.

This also adds asserts to both affected methods, to more explicitly catch any other cases where this could happen (in both cases, if those asserts fail, it's very likely that there'll be an NPE next stick in the corresponding finishX methods).

[nodejs-github-bot]

Review requested:

• @nodejs/net

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/53606/

[mcollina]

lgtm

[pimterry]

There's two CI failures here:

• node-test-commit-arm: sequential.test-single-executable-application-snapshot-and-code-cache
  That seems likely unrelated, and there's at least one other random failure of that test recently.
• win2012r2-vs2019-COMPILED_BY-vs2019-x86: InspectorSocketTest.HostIPv6NonRoutable
  As a networking test, that plausibly could be related, but it did also fail around the same time on another unrelated commit (here) so maybe not? I'm not sure how to reproduce that locally unfortunately. Would be good if somebody could rerun that, and I'll try to investigate it further if it's still failing.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/53639/

[pimterry]

Ok, that's somewhat clearer:

• The Windows tests now all pass happily, so that failure doesn't seem related
• An OSX test now fails for very unrelated reasons ("not enough disk space for writing...")
• The ARM test is still failing but now with a different test failing. Since that test passed last time, and doesn't seem to use networking at all, I think we can confidently say that's not related to this either.

Given that, I think that means this is good to go, and CI just needs nudging until it passes. I'll leave it here, but let me know if there's anything else I can do to help get this merged.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/53647/

[mcollina]

lgtm

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/49400
✔  Done loading data for nodejs/node/pull/49400
----------------------------------- PR info ------------------------------------
Title      Fix JS Stream Socket finishShutdown crash (#49400)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     pimterry:fix-js-stream-socket-finishShutdown-crash -> nodejs:main
Labels     author ready
Commits    2
 - net: fix crash due to simultaneous close/shutdown on JS Stream Sockets
 - net: use asserts in JS Socket Stream to catch races in future
Committers 1
 - Tim Perry 
PR-URL: https://github.com/nodejs/node/pull/49400
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/49400
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 29 Aug 2023 10:04:35 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/49400#pullrequestreview-1603592553
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/49400#pullrequestreview-1600355896
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-08-30T16:27:57Z: https://ci.nodejs.org/job/node-test-pull-request/53647/
- Querying data for job/node-test-pull-request/53647/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 49400
From https://github.com/nodejs/node
 * branch                  refs/pull/49400/merge -> FETCH_HEAD
✔  Fetched commits as b781eaf4309a..fb9611bd5d29
--------------------------------------------------------------------------------
[main 775d624f19] net: fix crash due to simultaneous close/shutdown on JS Stream Sockets
 Author: Tim Perry 
 Date: Thu Aug 24 16:05:02 2023 +0100
 2 files changed, 91 insertions(+)
 create mode 100644 test/parallel/test-http2-client-connection-tunnelling.js
[main 316fc7cb68] net: use asserts in JS Socket Stream to catch races in future
 Author: Tim Perry 
 Date: Fri Aug 25 14:16:35 2023 +0100
 1 file changed, 3 insertions(+)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

net: fix crash due to simultaneous close/shutdown on JS Stream Sockets
A JS stream socket wraps a stream, exposing it as a socket for something

on top which needs a socket specifically (e.g. an HTTP server).
If the internal stream is closed in the same tick as the layer on top

attempts to close this stream, the race between doShutdown and doClose

results in an uncatchable exception. A similar race can happen with

doClose and doWrite.
It seems legitimate these can happen in parallel, so this resolves that

by explicitly detecting and handling that situation: if a close is in

progress, both doShutdown & doWrite allow doClose to run

finishShutdown/Write for them, cancelling the operation, without trying

to use this._handle (which will be null) in the meantime.
PR-URL: #49400

Reviewed-By: Matteo Collina matteo.collina@gmail.com

Reviewed-By: Luigi Pinca luigipinca@gmail.com
[detached HEAD 189fd82de7] net: fix crash due to simultaneous close/shutdown on JS Stream Sockets

Author: Tim Perry pimterry@gmail.com

Date: Thu Aug 24 16:05:02 2023 +0100

2 files changed, 91 insertions(+)

create mode 100644 test/parallel/test-http2-client-connection-tunnelling.js

Rebasing (3/4)

Rebasing (4/4)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

net: use asserts in JS Socket Stream to catch races in future
PR-URL: #49400

Reviewed-By: Matteo Collina matteo.collina@gmail.com

Reviewed-By: Luigi Pinca luigipinca@gmail.com
[detached HEAD a803862f7d] net: use asserts in JS Socket Stream to catch races in future

Author: Tim Perry pimterry@gmail.com

Date: Fri Aug 25 14:16:35 2023 +0100

1 file changed, 3 insertions(+)
Successfully rebased and updated refs/heads/main.
ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/6035974708

[nodejs-github-bot]

Landed in f863117...47add7e