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 96.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 90.06%. Comparing base (debe2ed) to head (0984bca).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/http2/core.js	96.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #63414      +/-   ##
==========================================
+ Coverage   90.05%   90.06%   +0.01%     
==========================================
  Files         714      714              
  Lines      225742   225765      +23     
  Branches    42727    42723       -4     
==========================================
+ Hits       203285   203335      +50     
+ Misses      14234    14230       -4     
+ Partials     8223     8200      -23     

Files with missing lines	Coverage Δ
lib/internal/http2/core.js	95.19% <96.66%> (+<0.01%)	⬆️

... and 51 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));
	}
	}