search

tomas / needle

Nimble, streamable HTTP client for Node.js. With proxy, iconv, cookie, deflate & multipart support.
https://www.npmjs.com/package/needle
MIT License
1.62k stars 235 forks source link

fix: Silence ECONNRESET errors after connection close #392

Closed joeyparrish closed 2 years ago

joeyparrish commented 2 years ago

See also less/less.js#3693

Fixes #391

tomas commented 2 years ago

Thanks! Are you able to run all tests successfully?

joeyparrish commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

Yes sure. Thanks for the heads up.

tomas commented 2 years ago

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

image

tomas commented 2 years ago

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

image

This might also be relevant:

image

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

joeyparrish commented 2 years ago

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

joeyparrish commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Yup, there it is. Reverting this fixes it.

nvm exec 12 node node_modules/.bin/mocha test
joeyparrish commented 2 years ago

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.