http2: emit session close before stream close

[mcollina]

Fixes: #63412

This changes the HTTP/2 session shutdown path so that when the transport has
already been closed, the client session 'close' event is queued before stream
close callbacks are scheduled.

That gives clients a race-free chance to invalidate cached
ClientHttp2Session instances before stream 'close' handlers try to reuse
that session and synchronously hit ERR_HTTP2_INVALID_SESSION.

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[codecov]

Codecov Report

❌ Patch coverage is 90.90909% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.33%. Comparing base (debe2ed) to head (5fc94f7).
⚠️ Report is 144 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http2/core.js	90.90%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #63414      +/-   ##
==========================================
+ Coverage   90.05%   90.33%   +0.28%     
==========================================
  Files         714      730      +16     
  Lines      225742   234374    +8632     
  Branches    42727    43912    +1185     
==========================================
+ Hits       203285   211731    +8446     
- Misses      14234    14368     +134     
- Partials     8223     8275      +52     

Files with missing lines	Coverage Δ
lib/internal/http2/core.js	95.18% <90.90%> (-0.02%)	⬇️

... and 119 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[pimterry]

This changes the behaviour exclusively for this one case (where the underlying socket has actually been destroyed). I think there's quite a few other cases where you'll get very similar symptoms (e.g. GOAWAY, any other non-socket session issues that trigger session teardown) where you'll still hit the same event order, and so Undici would still hit the same issue even with this fix.

More generally, having the event ordering branch dependingh on error scenarios seems like it's going to be confusing - it'd be better to have a single ordering all the time. And the ordering is a bit surprising, I would normally expect streams within to close before sessions containing them... I worry that flipping the session/stream closure order will just expose some other race condition in the other direction somehow, since this change would now mean that session close would fire while every stream appeared to be open as normal.

Can we fix this in a cleaner way? What if we continue closing the streams first, like today (more intuitive imo, keeps single consistent ordering of events) but we emit all errors like this (any problems other than param validation) async instead of sync? Seems safe-ish, since async failure is clearly already possible and has to be handled anyway. There's similar other cases too already: if you call session.request while the session is still connecting, the request flow and any potential errors are deferred to async in all cases today.

That'd solve Undici's issue: you could still just listen to the events, and nothing will fail synchronously if you retry a new stream when one closes - it's just that the 2nd request will later emit 'error' (as it can in other cases today anyway).

That would also match HTTP/1 better. The code there explicitly pushes all initial sync connection issues to next tick:

node/lib/_http_client.js

Lines 420 to 441 in 2ebf533

	if (typeof opts.createConnection === 'function') {
	const oncreate = once((err, socket) => {
	if (err) {
	process.nextTick(() => emitErrorEvent(this, err));
	} else {
	this.onSocket(socket);
	}
	});
	try {
	const newSocket = opts.createConnection(opts, oncreate);
	if (newSocket) {
	oncreate(null, newSocket);
	}
	} catch (err) {
	oncreate(err);
	}
	} else {
	debug('CLIENT use net.createConnection', opts);
	this.onSocket(net.createConnection(opts));
	}
	}

[mcollina]

Thanks, I changed direction here based on the review.

Instead of reordering session/stream teardown, this now keeps the existing shutdown order and fixes the race by making ClientHttp2Session.request() defer invalid-session state errors to the returned stream.

So if user code retries from a stream 'close' callback while the cached session is already stale, it no longer throws synchronously with ERR_HTTP2_INVALID_SESSION. It returns a stream that emits ERR_HTTP2_INVALID_SESSION or ERR_HTTP2_GOAWAY_SESSION asynchronously, which gives session lifecycle handlers a chance to clear cached state first.

Updated in this branch:

• reverted the close-event ordering change in closeSession()
• changed request() to keep argument validation synchronous, but defer session-state failures asynchronously
• updated tests to cover the teardown/retry case and the destroy path
• updated the http2 docs for the new request() behavior

Validated locally with:

• make -j$(nproc)
• make lint-js LINT_JS_TARGETS='lib/internal/http2/core.js test/parallel/test-http2-client-destroy.js test/parallel/test-http2-client-session-close-before-stream-close.js'
• make lint-md LINT_MD_FILES='doc/api/http2.md'
• python3 tools/test.py test/parallel/test-http2-client-destroy.js test/parallel/test-http2-goaway-delayed-request.js test/parallel/test-http2-client-session-close-before-stream-close.js test/parallel/test-http2-server-shutdown-redundant.js

[mcollina]

Follow-up for the Darwin CI failures: I pushed a small test-only adjustment.

The implementation change looks fine, but test-http2-client-session-close-before-stream-close.js was too strict about the exact teardown state observed in the first stream 'close' callback. On the shared-library Darwin jobs, session.closed is already true there, but session.destroyed is not always observable yet.

The test now checks the behavior we actually care about:

• session.request() does not throw synchronously from the stream 'close' callback
• the returned stream reports the failure asynchronously
• cached session state has been cleared before that async error is handled

It now accepts either ERR_HTTP2_INVALID_SESSION or ERR_HTTP2_GOAWAY_SESSION for that callback timing.

[mcollina]

Another small Darwin follow-up pushed.

The failing assertion was still too strict about teardown timing in the first stream 'close' callback: even session.closed is not guaranteed to be observable there across all Darwin/shared-library configurations.

I removed that assertion as well, so the test is now focused only on the behavior we actually need to guarantee:

• session.request() must not throw synchronously from that callback
• the returned stream must fail asynchronously
• cached session state must be cleared before that async failure is handled

[pimterry]

Very nice, LGTM 👍

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in bac704d