http,https,http2: make async disposers idempotent

[LiviaMedeiros]

This PR makes server[Symbol.asyncDispose] calls not throw after the server is already closed.

{
  await using server = {http,https,http2}.createServer();
  server.listen();
  await server.close(); // close explicitly, throws if server is not running
} // close implicitly, does nothing if server is not running

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/http
• @nodejs/http2
• @nodejs/net

[jasnell]

Hmm does this make this a semver-major? This becomes a user observable change and if someone happens to be using an ERM polyfill I can imagine removing this breaking things.

[LiviaMedeiros]

Right, I forgot that disposers graduated from experimental state since v24.2.0.

[Renegade334]

Would it be better to split this into two patches, one to make the existing http* overrides idempotent, and another semver-major one to remove them?

[LiviaMedeiros]

I think since they are stable, making them not throw would technically already be a breaking change. If not, splitting is indeed better.
Another option is separate backport PRs changing only the overrides.

[bakkot]

In most cases making an error into a non-error is not a breaking change. Obviously it can be, but so can adding a new function or option.

(I don't know if node has a policy to the contrary, just speaking generally.)

[jasnell]

Yeah, we've generally treated removing throws as something less than semver-major, and adding a throw as always semver-major, but often it's a case-by-case determination.

[jasnell]

For this PR, rather than removing the asyncDispose, let's add a doc-only deprecation and make this one semver-patch.

Then, separately in another semver-major PR, we can runtime deprecate the asyncDispose for the next major. Then once 25.0.0 is cut later this year we can open another semver-major that moves it to EOL.

Painful, yes, I know.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.08%. Comparing base (faada65) to head (8136261).
Report is 12 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58832      +/-   ##
==========================================
- Coverage   90.09%   90.08%   -0.02%     
==========================================
  Files         640      640              
  Lines      188271   188356      +85     
  Branches    36923    36940      +17     
==========================================
+ Hits       169625   169675      +50     
- Misses      11386    11402      +16     
- Partials     7260     7279      +19     

Files with missing lines	Coverage Δ
lib/_http_server.js	97.05% <ø> (-0.02%)	⬇️
lib/https.js	99.29% <ø> (-0.01%)	⬇️
lib/internal/http2/core.js	95.15% <ø> (-0.01%)	⬇️
lib/net.js	95.28% <100.00%> (+<0.01%)	⬆️

... and 39 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.

[nodejs-github-bot]

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

[mcollina]

lgtm