Revert "http: do not emit socket errors after complete response"

[mcollina]

This reverts commit 17163ea / #64278.

This landed too quickly #64278 (comment). Alternative to #64507.

[nodejs-github-bot]

Review requested:

• @nodejs/http
• @nodejs/net

[github-actions]

Fast-track has been requested by @mcollina. Please 👍 to approve.

[mcollina]

@pimterry @lpinca PTAL

[pimterry]

Fair enough, I'm on board. #64507 has indeed hit CI issues with Mac, so it doesn't look like it's the easy tiny quick fix I was hoping. I'll it finish it up on top of this afterwards to bring the fix back correctly.

[Archkon]

Fair enough, I'm on board. #64507 has indeed hit CI issues with Mac, so it doesn't look like it's the easy tiny quick fix I was hoping. I'll it finish it up on top of this afterwards to bring the fix back correctly.

I kinda get to realize this, this maybe the problem with test code not nodejs lib code.Could you check my suggestion on #64507 thread? I think your fix is correct direction

[ShogunPanda]

LGTM!

[nodejs-github-bot]

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

[mcollina]

@Archkon can you please send this change again so we can reapply it/rediscuss it? Sorry.

[Archkon]

@Archkon can you please send this change again so we can reapply it/rediscuss it? Sorry.

I just think the problem with the test code where

assert.strictEqual(err.code, 'ECONNRESET');

in test-http-client-complete-response-open-request-reset.js

should be changed to like this

    assert.strictEqual(err.syscall, 'write');
    switch (err.code) {
    case 'ECONNRESET':
    case 'ECONNABORTED':
    case 'EPIPE':
      break;
    default:
      assert.fail(`Unexpected error code ${err.code}`);
  }

[Archkon]

@Archkon can you please send this change again so we can reapply it/rediscuss it? Sorry.

I just think the problem with the test code where

assert.strictEqual(err.code, 'ECONNRESET');

in test-http-client-complete-response-open-request-reset.js

should be changed to like this

    assert.strictEqual(err.syscall, 'write');
    switch (err.code) {
    case 'ECONNRESET':
    case 'ECONNABORTED':
    case 'EPIPE':
      break;
    default:
      assert.fail(`Unexpected error code ${err.code}`);
  }

Which you could find the similar test code in
test/parallel/test-http-destroyed-socket-write2.js

    switch (er.code) {
      // This is the expected case
      case 'ECONNRESET':
        break;

      // On Windows, this sometimes manifests as ECONNABORTED
      case 'ECONNABORTED':
        break;

      // This test is timing sensitive so an EPIPE is not out of the question.
      // It should be infrequent, given the 50 ms timeout, but not impossible.
      case 'EPIPE':
        break;

      default:
        // Write to a torn down client should RESET or ABORT
        assert.fail(`Unexpected error code ${er.code}`);
    }

I just guess that the macos would differ to respond a error code.

I was wrongly be confused this when I first met the macos test failed and I change checking whether response completed to just check whether response exist.That is my bad,sorry

[Archkon]

@pimterry @mcollina Could you check this and I have no macos device to test. Thanks!

@Archkon can you please send this change again so we can reapply it/rediscuss it? Sorry.

I just think the problem with the test code where

assert.strictEqual(err.code, 'ECONNRESET');

in test-http-client-complete-response-open-request-reset.js
should be changed to like this

    assert.strictEqual(err.syscall, 'write');
    switch (err.code) {
    case 'ECONNRESET':
    case 'ECONNABORTED':
    case 'EPIPE':
      break;
    default:
      assert.fail(`Unexpected error code ${err.code}`);
  }

Which you could find the similar test code in test/parallel/test-http-destroyed-socket-write2.js

    switch (er.code) {
      // This is the expected case
      case 'ECONNRESET':
        break;

      // On Windows, this sometimes manifests as ECONNABORTED
      case 'ECONNABORTED':
        break;

      // This test is timing sensitive so an EPIPE is not out of the question.
      // It should be infrequent, given the 50 ms timeout, but not impossible.
      case 'EPIPE':
        break;

      default:
        // Write to a torn down client should RESET or ABORT
        assert.fail(`Unexpected error code ${er.code}`);
    }

I just guess that the macos would differ to respond a error code.

I was wrongly be confused this when I first met the macos test failed and I change that to just check whether response exist.That is my bad,sorry

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.24%. Comparing base (7a11a9b) to head (fd66d5a).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #64511      +/-   ##
==========================================
- Coverage   90.24%   90.24%   -0.01%     
==========================================
  Files         739      739              
  Lines      241669   241655      -14     
  Branches    45543    45545       +2     
==========================================
- Hits       218087   218072      -15     
+ Misses      15150    15114      -36     
- Partials     8432     8469      +37     

Files with missing lines	Coverage Δ
lib/_http_client.js	97.61% <100.00%> (-0.03%)	⬇️

... and 36 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/74829/

[nodejs-github-bot]

Landed in 34c28d5