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

[Archkon]

@pimterry I think you could edit my PR branch, so could you add empty commit with only sign-off-by in commit messgae and I would squash into one commit beacuse I use the some test code authored by you?

[codecov]

Codecov Report

❌ Patch coverage is 74.41860% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.23%. Comparing base (89df046) to head (2818b98).
⚠️ Report is 3 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.23%   90.23%   -0.01%     
==========================================
  Files         739      739              
  Lines      241744   241813      +69     
  Branches    45564    45579      +15     
==========================================
+ Hits       218149   218194      +45     
- Misses      15115    15171      +56     
+ Partials     8480     8448      -32     

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.04% <75.00%> (-0.57%)	⬇️

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

[Archkon]

@pimterry Could you take a look at this and evaluate this pr proposal? Thanks !

[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';
const assert = require('assert');
const http = require('http');
const net = require('net');

// A server that never reads the request body and aborts the connection.
const server = net.createServer((socket) => socket.destroy());

server.listen(0, () => {
  const req = http.request({ port: server.address().port, method: 'POST' });
  let writeError = null;
  let finishEmitted = 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?

[Archkon]

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';
const assert = require('assert');
const http = require('http');
const net = require('net');

// A server that never reads the request body and aborts the connection.
const server = net.createServer((socket) => socket.destroy());

server.listen(0, () => {
  const req = http.request({ port: server.address().port, method: 'POST' });
  let writeError = null;
  let finishEmitted = 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?

Ok, but I was busy with other pr so I think this would be delayed a bit to solve

[pimterry]

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

[Archkon]

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

Fine,I hope this would push this pr forward smoothly