No new timeouts after an error

[szmarczak]

Fixes #631

[sindresorhus]

@timdp Can you try this out?

[szmarczak]

request.once('error', () => {}); // Ignore the socket hung up error made by request.abort()

Hmm... Is it a good idea to ignore all socket hung up errors? If users do promise.cancel(), they won't see them, unless they do promise.on('request', request => setTimeout(() => request.abort(), n))

[timdp]

@szmarczak I ran

yarn add 'szmarczak/got#fix-timeouts'

and the test from #631 worked as expected:

caught: { RequestError: getaddrinfo ENOTFOUND 1541424862699.dev 1541424862699.dev:80

However, it noticeably takes a while to execute. I added console.time() and consistently got execution times of just over two seconds. In the readme, it says that the time between retries is

1000 * Math.pow(2, retry) + Math.random() * 100

which, with retry = 0, means that the maximum execution time should be 1,100 ms plus a few milliseconds for the timeout, right? I don't get where the extra second goes.

[UltCombo]

@timdp I can confirm there's a bug in the default retry delay logic.

The retries function gets 1 for the iteration argument in the first retry (see here), so the retry delay math is wrong (1 << 1 === 2).

[timdp]

Indeed. The retry parameter starts at 1, not 0.

[timdp]

Also, I want to point out that the concept of cancelers is basically what RxJS used to call Disposable until version 4. We still have some code that depends on it at our company. Just another argument to support what I originally said about async state, even if it's not very pragmatic. :-)

[szmarczak]

@UltCombo @timdp

got/source/normalize-arguments.js

Lines 209 to 211 in 50fdab3

	if (iteration > retries) {
	return 0;
	}

with retry: 0 it should reject immediately. I'll check it tomorrow.

[timdp]

Yeah, but my original reproduction of the issue used retry: 1, so I kept that and discovered a second issue.

[szmarczak]

Yup. It's a bug. From the docs:

Delays between retries counts with function 1000 * Math.pow(2, retry) + Math.random() * 100, where retry is attempt number (starts from 0).

Let's merge this PR then. I'll make another one for that to keep things organized :)