Change timeout default to constant 80000 instead Node default

[jonchurch]

Closes #806

Set timeout default to a constant 80 seconds instead of using the default value of the version of Node.js the user is running.

Updated tests, docs, comment in types def file.

Note: Not sure how you define breaking changes here, but this would be a departure from the previous default behavior of 120 seconds. This issue is currently out in the wild though (for anyone *brave* enough to be using v13 in production) so I would suggest a release as soon as feasible.

Switching this back to a constant 120 seconds and a minor version bump would prevent anyone from encountering the issue in the current major.

[brandur-stripe]

Thanks for the thorough patch @jonchurch!

Going to take the risk here and release a minor change — our server side timeouts will almost certainly cut you off well well before you hit even 80 seconds, so I don't expect any user-discernible difference afterwards.

Just reading through Node docs, I also realize that we should also be setting a 30 second connect timeout when generating a request with https.request to be consistent with other libraries, but we can tackle that one another day.

[jonchurch]

Happy to help, and thank you for being so responsive ❤️.

Looking into this more (never realized there was this much to think about timeouts!) the current state of things for the library is that we are setting the timeout for idle TCP connections. Meaning the timeout will only be triggered after there is no activity over the connection for the duration of the timeout.

I think that setting the timeout when creating the request will share that timeout for all stages of the TCP socket's lifecycle: initial attempt at connecting, DNS lookup, after connection, between read/writes over the socket. The timeout is reset at each stage, and at each read/write event. Here's a decent blog post I found about this..

In the library the change would happen here, adding a timeout to the options, when the request is first created:

stripe-node/lib/StripeResource.js

Lines 417 to 425 in 1d6207e

	const req = (isInsecureConnection ? http : https).request({
	host: host || this._stripe.getApiField('host'),
	port: this._stripe.getApiField('port'),
	path,
	method,
	agent,
	headers,
	ciphers: 'DEFAULT:!aNULL:!eNULL:!LOW:!EXPORT:!SSLv2:!MD5',
	});

None of this really feels super high value IMO since the server will take care of timing out bad connections as you mentioned, but wanted to leave this comment to share what I learned.

[brandur-stripe]

NP!

the current state of things for the library is that we are setting the timeout for idle TCP connections. Meaning the timeout will only be triggered after there is no activity over the connection for the duration of the timeout.

I think you might have read the docs for net.Socket.setTimeout() instead of for request.setTimeout(timeout[, callback]). My reading of the latter makes it sound that our invocation of setTimeout does what we expect it to — namely to time out a request that we generate after after a certain duration.

In the library the change would happen here, adding a timeout to the options, when the request is first created:

Ah yep, that's where I was thinking. That timeout would have an effect on time to open the socket, which is 30 seconds for us in our other libraries. But yep, not it's also probably not a hugely critical change.

[jonchurch]

@brandonl-stripe Hmm I think I'm going further down the rabbit hole here than is productive but this has piqued my interest. Feel free to ignore this thread of conversation.

Under the hood request.setTimeout calls socket.setTimeout. It just waits until the connection event happens before setting the socket timeout:

https://github.com/nodejs/node/blob/7c524fb092b143470795c8ca869de4654c728fe1/lib/_http_client.js#L797-L823

I realize now my source of misunderstanding. I was thinking of timeouts in terms of how long it took for the entire response to be received. I think traditionally HTTP timeouts refer to how long it takes to receive a response, not how long it takes to receive the entire response.

The above timeout affects how long a connection will wait to hear back from the server, not how long it takes to receive the entire response body. Since this library talks to the Stripe API and not a potentially bad actor very slowly sending the response, my interpretation was irrelevant.

After connection, while waiting for the response to be sent from the server, the socket is idle. So if the response doesn't begin flowing before the set interval, we see the timeout.

You can also force a timeout if there is X time between packets sent from the server after the response has begun flowing if you reset the timeout after receiving the response.

Here's a gist where I thought through all this. I picked a really slow API (the NASA Exoplanet API).

[brandonl-stripe]

@brandur-stripe ☝️ 😄