Review requested:

• @nodejs/http
• @nodejs/net

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.03%. Comparing base (0b5b189) to head (a098f1e).
⚠️ Report is 165 commits behind head on main.

@@            Coverage Diff             @@
##             main   #63448      +/-   ##
==========================================
- Coverage   90.06%   90.03%   -0.03%     
==========================================
  Files         714      714              
  Lines      225918   225930      +12     
  Branches    42734    42735       +1     
==========================================
- Hits       203464   203422      -42     
- Misses      14232    14281      +49     
- Partials     8222     8227       +5     

... and 34 files with indirect coverage changes

• ❄️ 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.

We need to link this up to #63249, which actively aims to match the current ServerResponse behaviour that this PR is now changing (neither erroring nor finishing if clients abort). These behaviours need to match.

Do we definitely want to change this though? In the referenced issue (#50656) back in 2023, @ronag said:

The current invariant for Writable streams is that they should always emit either 'error' or 'finish'.

But that's not generally true. There's plenty of counter-examples, most notably calling destroy() on any writables always emits close without error or finish. Similarly, we don't error or finish stdin if a child process cleanly exits:

const { spawn } = require('node:child_process');

const child = spawn('node', ['-e', 'process.exit(0)']);

child.stdin.on('finish', () => console.log('stdin: finish'));
child.stdin.on('error', () => console.log('stdin: error'));
child.stdin.on('close', () => console.log('stdin: close'));

In effect, closed && !errored && !writableFinished is our standard 'clean abort' check. This is sufficiently standard that we check for it explicitly here as part of stream.finished() - automatically turning any such clean abort into a ERR_STREAM_PREMATURE_CLOSE if you're actively waiting for finished as a promise.

In the specific HTTP case this is a very common occurrence (clients abort responses all the time) so this will produce a lot of errors. It's good that it's guarded just for listened events, but still it'll be a very visible behavioural change and it's hard to predict the results of that. Currently, AFAICT most implementations largely happily ignore this scenario unless they're doing something expensive, and they can already detect this manually if they want to.

I'm very open to exploring this more, we could certainly improve stream & HTTP client abort behaviour, but this isn't the existing invariant, and I can see arguments on it both ways so we shouldn't just accept it as given. If we really do want to change it, we should definitely change the HTTP/2 PR above before that's merged too, and then potentially look at changing lots of other Node APIs as well afterwards to try to make this invariant consistent.

Any thoughts on the above @trivikr or @ronag? #63249 is basically ready to merge, but I'm currently holding on that given the open question here about how we want these APIs to work in general.

Personally, I think the current behaviour makes sense (clean abort of a writable = close without finish or error) and is consistent with the rest of our stream APIs. I'd suggest we stick with it, and potentially document better if there are gaps there. Let me know what you think?

@ronag Do you have any input on questions shared by @pimterry?

I'd posted this PR on coming across #50656 when searching for issues which mentioned "stream"

Ok, no specific objections here, so I'm going to go ahead with #63249 for now.

Happy to discuss and revisit the close-without-finish/error behaviour later on if there's appetite, but right now I do think that's our standard behaviour, and it's a reasonable solution.