fix: Silence ECONNRESET errors after connection close

[joeyparrish]

See also less/less.js#3693

Fixes #391

[tomas]

Thanks! Are you able to run all tests successfully?

[joeyparrish]

I get some test failures, but it's the same tests with and without my change:

  295 passing (10s)
  2 pending
  7 failing

  1) character encoding
       Given content-type: "text/html; charset=EUC-JP"
         with decode = false
           does not decode:
     Uncaught TypeError: Cannot read properties of undefined (reading 'body')
      at /home/joey/hacking/work/forks/needle/test/decoder_spec.js:23:16
      at done (lib/needle.js:493:14)
      at ClientRequest.had_error (lib/needle.js:508:5)
      at ClientRequest.emit (node:events:390:28)
      at Socket.socketErrorListener (node:_http_client:447:9)
      at Socket.emit (node:events:390:28)
      at emitErrorNT (node:internal/streams/destroy:157:8)
      at emitErrorCloseNT (node:internal/streams/destroy:122:3)
      at processTicksAndRejections (node:internal/process/task_queues:83:21)

  2) character encoding
       Given content-type: "text/html; charset=EUC-JP"
         with decode = true
           decodes:
     Uncaught TypeError: Cannot read properties of undefined (reading 'body')
      at /home/joey/hacking/work/forks/needle/test/decoder_spec.js:38:16
      at done (lib/needle.js:493:14)
      at ClientRequest.had_error (lib/needle.js:508:5)
      at ClientRequest.emit (node:events:390:28)
      at Socket.socketErrorListener (node:_http_client:447:9)
      at Socket.emit (node:events:390:28)
      at emitErrorNT (node:internal/streams/destroy:157:8)
      at emitErrorCloseNT (node:internal/streams/destroy:122:3)
      at processTicksAndRejections (node:internal/process/task_queues:83:21)

  3) request stream length
       no stream_length set
         returns 400 if Transfer-Encoding is set to a blank string:

      Uncaught AssertionError: expected 200 to equal 400
      + expected - actual

      -200
      +400
      
      at Assertion.fail (node_modules/should/cjs/should.js:275:17)
      at Assertion.value (node_modules/should/cjs/should.js:356:19)
      at /home/joey/hacking/work/forks/needle/test/request_stream_spec.js:66:34
      at done (lib/needle.js:493:14)
      at PassThrough.<anonymous> (lib/needle.js:756:9)
      at PassThrough.emit (node:events:402:35)
      at endReadableNT (node:internal/streams/readable:1343:12)
      at processTicksAndRejections (node:internal/process/task_queues:83:21)

  4) request stream length
       stream_length set to invalid value
         returns 400 if Transfer-Encoding is set to a blank string:

      Uncaught AssertionError: expected 200 to equal 400
      + expected - actual

      -200
      +400
      
      at Assertion.fail (node_modules/should/cjs/should.js:275:17)
      at Assertion.value (node_modules/should/cjs/should.js:356:19)
      at /home/joey/hacking/work/forks/needle/test/request_stream_spec.js:96:34
      at done (lib/needle.js:493:14)
      at PassThrough.<anonymous> (lib/needle.js:756:9)
      at PassThrough.emit (node:events:402:35)
      at endReadableNT (node:internal/streams/readable:1343:12)
      at processTicksAndRejections (node:internal/process/task_queues:83:21)

  5) request stream length
       stream_length set to invalid value
         returns 400 if Transfer-Encoding is not set:

      Uncaught AssertionError: expected 200 to equal 400
      + expected - actual

      -200
      +400
      
      at Assertion.fail (node_modules/should/cjs/should.js:275:17)
      at Assertion.value (node_modules/should/cjs/should.js:356:19)
      at /home/joey/hacking/work/forks/needle/test/request_stream_spec.js:104:34
      at done (lib/needle.js:493:14)
      at PassThrough.<anonymous> (lib/needle.js:756:9)
      at PassThrough.emit (node:events:402:35)
      at endReadableNT (node:internal/streams/readable:1343:12)
      at processTicksAndRejections (node:internal/process/task_queues:83:21)

  6) request stream length
       stream_length set to 0
         stream without path
           returns 400 if Transfer-Encoding is set to a blank string:

      Uncaught AssertionError: expected 200 to equal 400
      + expected - actual

      -200
      +400
      
      at Assertion.fail (node_modules/should/cjs/should.js:275:17)
      at Assertion.value (node_modules/should/cjs/should.js:356:19)
      at /home/joey/hacking/work/forks/needle/test/request_stream_spec.js:213:36
      at done (lib/needle.js:493:14)
      at PassThrough.<anonymous> (lib/needle.js:756:9)
      at PassThrough.emit (node:events:402:35)
      at endReadableNT (node:internal/streams/readable:1343:12)
      at processTicksAndRejections (node:internal/process/task_queues:83:21)

  7) socket cleanup
       should cleanup sockets on ERR_STREAM_PREMATURE_CLOSE (using stream.pipeline):

      Uncaught AssertionError: expected 1 to equal 0
      + expected - actual

      -1
      +0
      
      at Assertion.fail (node_modules/should/cjs/should.js:275:17)
      at Assertion.value (node_modules/should/cjs/should.js:356:19)
      at Timeout._onTimeout (test/socket_cleanup_spec.js:73:33)
      at listOnTimeout (node:internal/timers:557:17)
      at processTimers (node:internal/timers:500:7)

[joeyparrish]

@tomas, since the test failures I'm seeing do not appear to be regressions, can we merge this change? Less.js is waiting on this fix to update their dependency. Without this fix, less v4 is broken on macOS.

[tomas]

Yes sure. Thanks for the heads up.

[tomas]

Hold on, this is breaking the tests in socket_pool_spec.js: (Node v12.21.0)

[tomas]

This seems to fix the issue, but I'm not sure if it solves #392.

This might also be relevant:

I don't remember exactly why I commented that part of the code, but it seems related.

[joeyparrish]

I'll test on mac and see if the issue is still fixed with that change.

[joeyparrish]

It looks like that change would reintroduce the ECONNRESET bug. request.socket.destroyed seems to always be false when that code is executed.

If you disable my fix, the bug I'm fixing seems to be triggered by one of the existing tests on macOS, FWIW:
./node_modules/.bin/mocha --grep EPIPE test

  1) when posting a very long string
       shouldn't throw an EPIPE error out of nowhere:
     Uncaught Error: read ECONNRESET
      at WriteWrap.onWriteComplete [as oncomplete] (node:internal/stream_base_commons:94:16)

I don't see any of the socket reuse tests failing, for some reasons, neither on macOS nor on Linux. But I see many test failures with or without my changes, and they are inconsistent and different across operating systems.

Does limiting my workaround to macOS solve the test failure for you? (Since I can't reproduce the failure, I can't say if this is helpful or not, but I'm assuming we have some OS-specific difference at play.)

if (process.platform == 'darwin') request.abort();

[joeyparrish]

I filed issues for the tests that fail on my mac and Linux workstations with node v16, and I uploaded PRs to fix them. I'll try node v12 on Linux via nvm and see if I can reproduce that reuse issue.

[joeyparrish]

Yup, there it is. Reverting this fixes it.

nvm exec 12 node node_modules/.bin/mocha test

[joeyparrish]

I have PRs up to fix all the various test failures I've seen, including the reuse issue you found. With those, I have tests passing on Linux and macOS on node v4, v6, v8, v10, v12, v14, v16, and v17.