Got: Internal "options.timeout" is incompatible with the "timeout" option of "http.ClientRequest"

[kettanaito]

Describe the bug

• Node.js version: v12.18.0
• OS & version: MacOS Catalina (0.15.5 (19F101))

Actual behavior

Hey 👋 I'm one of the maintainers of MSW, an API mocking library. Some of our users have reported a type error when using MSW and Got for requests interception in NodeJS (see https://github.com/mswjs/node-request-interceptor/issues/86). I've investigated the issue and tracked it down to got source code, with which I'd kindly ask for your help.

The internal options.timeout seems to represent a map of differents timeouts ("request", "socket", "lookup", "connect", etc.), which can be observed in how that option is handled when it's passed to the timed-out.js utility as the delays argument.

Here's the journey of the options.timeout property:

https://github.com/sindresorhus/got/blob/8b88be2c66d3c7d3619df283eb2d852164b92259/source/index.ts#L43

During the _makeRequest function the timeout option gets deleted and restored:

https://github.com/sindresorhus/got/blob/8b88be2c66d3c7d3619df283eb2d852164b92259/source/core/index.ts#L2421-L2424

https://github.com/sindresorhus/got/blob/8b88be2c66d3c7d3619df283eb2d852164b92259/source/core/index.ts#L2364-L2366

Eventually, it gets passed to the timed-out.js:

https://github.com/sindresorhus/got/blob/8b88be2c66d3c7d3619df283eb2d852164b92259/source/core/index.ts#L2190

And handled depending on what keys options.timeout has:

https://github.com/sindresorhus/got/blob/8b88be2c66d3c7d3619df283eb2d852164b92259/source/core/utils/timed-out.ts#L98-L102

options.timeout becomes delays.

I don't yet understand why, but the internal options object of got is passed to the ClientRequest constructor at some point of time. I can see that because options object of our internal request interception library changes its options.timeout value from undefined to {} (set by got) during runtime:

https://github.com/mswjs/node-request-interceptor/blob/2f5a6458f41ce1b3e83eabc15a59778579d8155d/src/interceptors/ClientRequest/ClientRequestOverride.ts#L35

When ClientRequest is constructed with{ timeout: {} } option that causes a type error:

TypeError [ERR_INVALID_ARG_TYPE]: The "timeout" argument must be of type number. Received an instance of Object
    at validateNumber (internal/validators.js:125:11)
    at getTimerDuration (internal/timers.js:376:3)
    at new ClientRequest (_http_client.js:167:20)
    at request (http.js:46:10)

That behavior is according to NodeJS spec, as options.timeout must be a number:

timeout <number>: A number specifying the socket timeout in milliseconds. This will set the timeout before the socket is connected.
—https://nodejs.org/docs/latest-v12.x/api/http.html#http_http_request_options_callback

Expected behavior

I haven't dug that deep into got, but I suspect that it uses the internal options object to construct a ClientRequest instance. If that's the case, I think the options.timeout property should be renamed to represent the internal timeouts map, and not the ClientRequest's timeout options per specification.

I'd be thankful for your assistance on this.

Code to reproduce

• Checkout this reproduction repo.
• yarn install
• node src/index.js

Checklist

• [x] I have read the documentation.
• [x] I have tried my code with the latest version of Node.js and Got.

[szmarczak]

Looks like a race condition in your project. requestOptions.timeout is undefined when running

console.log(requestOptions.timeout);

right before

https://github.com/sindresorhus/got/blob/8b88be2c66d3c7d3619df283eb2d852164b92259/source/core/index.ts#L2415

szm@solus ~/Desktop/got $ npm run build && node demo.js

> [email protected] build
> del-cli dist && tsc

undefined
node:internal/process/promises:225
          triggerUncaughtException(err, true /* fromPromise */);
          ^

TypeError [ERR_INVALID_ARG_TYPE]: The "timeout" argument must be of type number. Received an instance of Object
    at new NodeError (node:internal/errors:278:15)
    at validateNumber (node:internal/validators:128:11)
    at getTimerDuration (node:internal/timers:383:3)
    at new ClientRequest (node:_http_client:173:20)
    at request (node:http:50:10)
    at Object.proxiedOriginalRequest (/home/szm/Desktop/got/node_modules/node-request-interceptor/lib/interceptors/ClientRequest/index.js:55:36)
    at ClientRequestOverride.<anonymous> (/home/szm/Desktop/got/node_modules/node-request-interceptor/lib/interceptors/ClientRequest/ClientRequestOverride.js:249:39)
    at step (/home/szm/Desktop/got/node_modules/node-request-interceptor/lib/interceptors/ClientRequest/ClientRequestOverride.js:33:23)
    at Object.next (/home/szm/Desktop/got/node_modules/node-request-interceptor/lib/interceptors/ClientRequest/ClientRequestOverride.js:14:53)
    at fulfilled (/home/szm/Desktop/got/node_modules/node-request-interceptor/lib/interceptors/ClientRequest/ClientRequestOverride.js:5:58) {
  code: 'ERR_INVALID_ARG_TYPE'
}

[szmarczak]

That's right. If I remove

https://github.com/sindresorhus/got/blob/8b88be2c66d3c7d3619df283eb2d852164b92259/source/core/index.ts#L2423

and make delays default to {}

https://github.com/sindresorhus/got/blob/8b88be2c66d3c7d3619df283eb2d852164b92259/source/core/utils/timed-out.ts#L45

then I get

error RequestError: connect ECONNREFUSED 127.0.0.1:80

which assume is expected.

[szmarczak]

I guess you don't clone the options, therefore modifying them results in an unexpected error. No such thing happens when running native Node.js.

[szmarczak]

The culprit is here. You forgot to clone the options.

https://github.com/mswjs/node-request-interceptor/blob/2f5a6458f41ce1b3e83eabc15a59778579d8155d/src/interceptors/ClientRequest/utils/normalizeHttpRequestParams.ts#L66

[kettanaito]

Thank you for the suggestions, @szmarczak! Cloning the request options indeed helped.

The behavior after cloning them, although doesn't involve options.timeout anymore, isn't valid. This exception is not expected:

error RequestError: connect ECONNREFUSED 127.0.0.1:80

Somewhere along with the request life-cycle, there's a request to 127.0.0.1:80, although it's never described explicitly. The code is making a request to http://localhost:<RANDOM_PORT> and I can confirm that request options describe that URL correctly.

If I console.log(options.url) in _makeRequest, I see the correct URL: http://localhost:54765/user. Nevertheless, the exception is still thrown. If I swap got with node-fetch without any extra change the exception is gone and the request is correctly made to the test server on the right port.

+import fetch from 'node-fetch'

-got(server.makeHttpUrl('/user'))
+fetch(server.makeHttpUrl('/user'))

server.makeHttpUrl returns a string that represents a URL relative to the spawned test server.

Can it be that got performs an intermediate request to :80 for some reason?

[Giotino]

Your repo name is a red flag here: mswjs/node-request-interceptor

Are you modifying the default Node.JS request function (or Node.JS HTTP Agent)?

[Giotino]

I think I found the answer I was looking for

This library monkey-patches the following native modules:

http.get/http.request
https.get/https.request

https://github.com/mswjs/node-request-interceptor/blob/master/src/interceptors/ClientRequest/ClientRequestOverride.ts

[kettanaito]

@Giotino, I believe you've found the correct answer. That repository is a library that provisions request interception in NodeJS. It works similarly to Nock, only with the inversed control over the mocked responses. I wish there was a better way to intercept requests in Node, but monkey-patching native modules is the only way there is now.

That being said, we model the intercepted behavior as close to the original one as possible, diving into the source code ofhttp.ClientRequest and replicating that to the necessary degree. We also don't override each and every aspect of the native modules, only the parts necessary to enable the interception.

Modification of the native modules doesn't seem to be affecting anything for this issue: as I've mentioned above, the code works fine using node-fetch or plain http.ClientRequest. It makes a logical assumption that unexpected behavior (_not_ necessarily the root cause) is related to got, so I'm reaching for help.

[szmarczak]

https://github.com/mswjs/node-request-interceptor/pull/87#issuecomment-803506678

[szmarczak]

@sindresorhus @Giotino I have investigated and it turns out the bug is on the interceptor side.

[kettanaito]

Once more huge thanks to @szmarczak. The issue has been investigated and resolved. No changes on got side are necessary, entirely scoped to the interceptor.