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 mswjs/interceptors#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:

got/source/index.ts

Line 43 in 8b88be2

timeout: {},

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

got/source/core/index.ts

Lines 2421 to 2424 in 8b88be2

	// Restore options
	options.request = request;
	options.timeout = timeout;
	options.agent = agent;

got/source/core/index.ts

Lines 2364 to 2366 in 8b88be2

	// TODO: Fix this ignore.
	// @ts-expect-error
	delete options.timeout;

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

got/source/core/index.ts

Line 2190 in 8b88be2

this[kCancelTimeouts] = timedOut(request, timeout, url);

And handled depending on what keys options.timeout has:

got/source/core/utils/timed-out.ts

Lines 98 to 102 in 8b88be2

	if (typeof delays.request !== 'undefined') {
	addTimeout(delays.request, timeoutHandler, 'request');
	}
	if (typeof delays.socket !== 'undefined') {

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

• I have read the documentation.
• 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

got/source/core/index.ts

Line 2415 in 8b88be2

let requestOrResponse = await fn(url, requestOptions);

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

> got@11.8.0 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

got/source/core/index.ts

Line 2423 in 8b88be2

options.timeout = timeout;

and make delays default to {}

got/source/core/utils/timed-out.ts

Line 45 in 8b88be2

export default (request: ClientRequest, delays: Delays, options: TimedOutOptions): () => void => {

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]

mswjs/interceptors#87 (comment)

[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.

[Drarig29]

For future reference, the delete options.timeout; line was removed in #1667, which was released in Got 12.