sindresorhus / got

🌐 Human-friendly and powerful HTTP request library for Node.js
MIT License
14.27k stars 935 forks source link

Regression when used with nock #1189

Closed lpinca closed 4 years ago

lpinca commented 4 years ago

Describe the bug

got@11 introduced a regression when used in a testing environment with nock.

Code to reproduce

const assert = require('assert');
const got = require('got');
const nock = require('nock');

nock('http://example.com').get('/test').delayBody(200).reply(200, 'foo');

got('http://example.com/test', { timeout: 100, retry: 0 })
  .then(function () {
    throw new Error('Test invalidation');
  })
  .catch(function (err) {
    assert.strictEqual(err.constructor, got.TimeoutError);
  });

Run with node --unhandled-rejections=strict index.js

Actual behavior

$ node --unhandled-rejections=strict index.js 
events.js:292
      throw er; // Unhandled 'error' event
      ^

Error: socket hang up
    at InterceptedRequestRouter.handleAbort (/Users/luigi/test-case/node_modules/nock/lib/intercepted_request_router.js:200:28)
    at OverriddenClientRequest.req.abort (/Users/luigi/test-case/node_modules/nock/lib/intercepted_request_router.js:79:35)
    at PromisableRequest._destroy (/Users/luigi/test-case/node_modules/got/dist/source/core/index.js:1013:32)
    at PromisableRequest.destroy (internal/streams/destroy.js:36:8)
    at PromisableRequest.<anonymous> (/Users/luigi/test-case/node_modules/got/dist/source/as-promise/index.js:194:25)
    at Object.onceWrapper (events.js:422:26)
    at PromisableRequest.emit (events.js:327:22)
    at PromisableRequest._beforeError (/Users/luigi/test-case/node_modules/got/dist/source/as-promise/core.js:115:14)
    at OverriddenClientRequest.<anonymous> (/Users/luigi/test-case/node_modules/got/dist/source/core/index.js:761:18)
    at Object.onceWrapper (events.js:422:26)
Emitted 'error' event on OverriddenClientRequest instance at:
    at /Users/luigi/test-case/node_modules/nock/lib/intercepted_request_router.js:108:11
    at processTicksAndRejections (internal/process/task_queues.js:79:11) {
  code: 'ECONNRESET'
}

Expected behavior

The process exits cleanly with no errors. This is what happens with got@10.

Checklist

szmarczak commented 4 years ago

What is the result on Node.js 13?

lpinca commented 4 years ago

The same

$ docker run -it --rm -v "$PWD":/tmp/test -w /tmp/test node:13-alpine node --unhandled-rejections=strict index.js 
events.js:292
      throw er; // Unhandled 'error' event
      ^

Error: socket hang up
    at InterceptedRequestRouter.handleAbort (/tmp/test/node_modules/nock/lib/intercepted_request_router.js:200:28)
    at OverriddenClientRequest.req.abort (/tmp/test/node_modules/nock/lib/intercepted_request_router.js:79:35)
    at PromisableRequest._destroy (/tmp/test/node_modules/got/dist/source/core/index.js:1013:32)
    at PromisableRequest.destroy (internal/streams/destroy.js:55:8)
    at PromisableRequest.<anonymous> (/tmp/test/node_modules/got/dist/source/as-promise/index.js:194:25)
    at Object.onceWrapper (events.js:422:26)
    at PromisableRequest.emit (events.js:327:22)
    at PromisableRequest._beforeError (/tmp/test/node_modules/got/dist/source/as-promise/core.js:115:14)
    at OverriddenClientRequest.<anonymous> (/tmp/test/node_modules/got/dist/source/core/index.js:761:18)
    at Object.onceWrapper (events.js:422:26)
Emitted 'error' event on OverriddenClientRequest instance at:
    at /tmp/test/node_modules/nock/lib/intercepted_request_router.js:108:11
    at processTicksAndRejections (internal/process/task_queues.js:79:11) {
  code: 'ECONNRESET'
}

Works as expected with got@10.

szmarczak commented 4 years ago

This is actually a bug in Nock. Calling .abort() after .destroy() should not throw an error. But I'll be happy to implement a fix on the Got side for the time being.

szmarczak commented 4 years ago

Unfortunately I cannot fix it in Got because request.destroyed is undefined.

szmarczak commented 4 years ago

Fixed in nock@13.0.0-beta.2.