http: fix the reverted commit which would swallow some errors

[Archkon]

Alternative PR to follow-up #64507

Follow-up to: #64507
Original PR Refs: #64278
Fixes: #64272
Refs:#64511
Refs: libuv/libuv#5196
Refs: #64507 (comment)
Refs: #64511 (comment)

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net
• @nodejs/streams

[codecov]

Codecov Report

❌ Patch coverage is 74.41860% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.28%. Comparing base (2ddade6) to head (dcbb130).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/_http_client.js	75.00%	8 Missing ⚠️
lib/internal/stream_base_commons.js	66.66%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #64566 +/- ##
==========================================
- Coverage 90.28% 90.28% -0.01% 
==========================================
Files 762 762 Lines 247646 247687 +41 Branches 46695 46705 +10 ==========================================
+ Hits 223596 223623 +27 
Misses 15496 15496 - Partials 8554 8568 +14 

Files with missing lines	Coverage Δ
lib/internal/streams/utils.js	97.80% <100.00%> (+0.01%)	⬆️
lib/internal/stream_base_commons.js	95.25% <66.66%> (-0.92%)	⬇️
lib/_http_client.js	97.08% <75.00%> (-0.56%)	⬇️

... and 35 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]

Sorry I haven't reviewed this yet @Archkon. I think it does match the description before, but I did some digging and I think there's an existing bug with writableFinished, which means even though that's correct we can't enable this yet... We'll need to fix that as well to ship this. Demo:

'use strict';constassert=require('assert');consthttp=require('http');constnet=require('net');// A server that never reads the request body and aborts the connection.constserver=net.createServer((socket)=>socket.destroy());server.listen(0,()=>{constreq=http.request({port: server.address().port,method: 'POST'});letwriteError=null;letfinishEmitted=false;req.on('finish',()=>{finishEmitted=true;});req.on('error',()=>{});req.on('close',()=>{server.close();// The write failed, so the body was never flushed to the socket.assert.strictEqual(writeError.code,'EPIPE');assert.strictEqual(req.writableFinished,false,'writableFinished is true after a failed write');assert.strictEqual(finishEmitted,false,"'finish' was emitted after a failed write");});// 1 MiB, so the write cannot fit in the socket buffers and must fail.req.write(Buffer.alloc(1024*1024),(err)=>{writeError=err;});req.end();});

If you tweak your tests slightly you'll hit the same thing. This seems to be an existing bug, but it doesn't normally matter because error emits first and we don't base anything on writableFinished. Once we do, this will swallow write errors just like before. Do you have a little time to take a look?

[pimterry]

I've opened a separate PR to fix writableFinished: #64847. Once that's merged, I think this will work correctly.

[pimterry]

#64847 is now merging any second (just waiting for the commit queue). As soon as that's on main, you can rebase this and then hopefully the above example will now work correctly and we can get this merged too.

[pimterry]

@Archkon This is quite confusingly written, so I'm not really clear what you mean... In future I'd suggest maybe providing examples or a larger explanation.

For now I can clarify the general invariant though, which might help: we want to preserve the error handling behaviour from 24.15.

For the current test, and any other test you can write, we should see the same error behaviour with this fix as we did in 24.15 before the regression.

If you find that the core fix behaves the same as 24.15 in every case for this issue then it's correct I think, we're happy with that original behaviour. We only need to change the core code if you find a difference between this and 24.15.

If the code is correct but the test here is failing, then that must mean the test was also failing in 24.15. Is that right? If so then yes, in that case we should fix the test.

Overall I think we need two tests (one for read errors and one for write errors) which both pass in 24.15, fail in 24.16, and then pass with this fix. They need a configuration that does that consistently on both Linux & Mac.

Does that help?