Feature: Timeout, Retry, and Queue

[niftylettuce]

I'm writing my thoughts down here, because I do not have the time right now to dedicate to adding this, however hopefully someone in the community might be able to. I'm closing #66, #69, #73 in favor of this single issue.

cc @kasbah @akmjenkins @davidgovea

Retry

We should not retry on all 500 status error codes as every other fetch retry package does - instead we should retry only based off of it isRetryAllowed(err) returns true by using is-retry-allowed OR if the response had a 429, 502, 503, or 504 status code.

We should also only retry on GET, PUT, HEAD, DELETE, OPTIONS, or TRACE methods. Our approach should also respect the Retry-After response header (see https://github.com/sindresorhus/got/blob/6eaa81ba8db87340bbdbd79ef91594803f37b854/source/normalize-arguments.ts#L231-L241 for inspiration.

In general our approach will be very similar to got and ky (e.g. https://github.com/sindresorhus/ky).

It should be up to the application layer to retry if a successful server response status code occurs, and it should not be fetch nor frisbee responsibility to retry based off a server response (which is why we will not use packages like fetch-retry-or-die, make-fetch-happen, nor fetch-retry (because all of these implementations retry on 5xx status codes and none of them respect the actual error code). If a server responds, it's considered to be successful.

Finally, if someone aborts a Frisbee fetch request using AbortController, then the retries should be aborted as well and it should not retry from an AbortError. This should happen automatically out of the box, but I wanted to write it down. The best way to detect abort errors is probably the same as we do with tests, via err.name === 'AbortError' (see https://github.com/niftylettuce/frisbee/blob/master/test/node.test.js#L334).

I think that we can use the p-retry package and allow a global retry option via new Frisbee({ retry: { ... } }), and for individual API methods such as frisbee.get('/', { retry: { ... } }) as well. If an individual API method specifies a retry object, then p-retry should use that, otherwise if a global option was passed it will use it. There are several other packages that are very similar (most of which use node-retry as well), however I trust @sindresorhus and his dedication to open source maintenance and quality, therefore I would use p-retry above all.

We should also respect the Retry-After header per https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After for 429 status codes.

Timeout

A timeout event should cause a retry, therefore we need to make sure the error we throw on a timeout (namely its code) returns true from isRetryAllowed(err). To properly design this, we should either use an existing AbortController passed, or create a new AbortController for each request. If the timeout is triggered, then it would signal an abort with an error created via const error = new Error('ETIMEDOUT'); error.code = 'ETIMEDOUT'; (similar to how request does it here https://github.com/request/request/blob/df346d8531ac4b8c360df301f228d5767d0e374e/request.js#L848-L849). By using AbortController, we can ensure that the fetch attempted is actually aborted, and will not be completed/return a response after the timeout error occurs.

Unfortunately we cannot use any existing fetch timeout packages because all of them polyfill or have odd implementations of detecting which fetch implementation to use, therefore we have to write this ourselves.

We can most likely use p-timeout for this.

[niftylettuce]

Also, per #92 we should allow a custom callback function to be called on retries, which would allow JWT refresh tokens to be updated for example.

[akmjenkins]

My fetch-retry approach was to leave it entirely up to the user (no opinions based on server response codes). It could be passed into a base frisbee instance or individual requests. The API was pretty straightforward:

{
    retries: number of times to retry,
    retryOn: [array of status codes],
    retryFn?: async function to execute between each retry
    retryTimeout?: ms to wait in between retries (sensible default - like 1000ms, ignored if retryFn is supplied)
}

The only tricky parts were those you pointed out - don't retry if the AbortController.signal.aborted === true

The retryFn was especially helpful because it allows the user to do absolutely anything they want (like refresh JWT) in between retries.

Would an API like this work?

[niftylettuce]

I like the simplicity of that - but I think that you could eliminate retryOn and have that logic be up to the user in retryFn. Then we could make a default retryFn that checks against isRetryAllowed was true OR if status code was 429, 502, 503, or 504, AND ALSO checks against the Retry-After header if it existed.

[antgel]

Great to see the activity around here! :) I don't mind that #86 was closed in favour of this, but does the above issue description include relevant details to handle that scenario? (Retrying a request after refreshing JWT access token.)

[niftylettuce]

Yeah @antgel, you would be able to refresh JWT token in retryFn.

@akmjenkins I'll gladly award you the bounty if you can open a PR for this https://github.com/niftylettuce/bug-bounties/issues/5

[antgel]

Oops, I missed it, it was in #92 (comment). :)

[niftylettuce]

We should also use https://github.com/sindresorhus/p-queue to create a queue of requests that can be processed.

[niftylettuce]

Just FYI I already implemented timeout and retry locally. Just need to push it up. I definitely need some help with tests.

[niftylettuce]

I pushed my work in progress by the way at 38d2252 if anyone wants to help out with it.

I would love help writing tests for it. Timeouts/retries all need tested.

[niftylettuce]

What's left:

• Write tests for timeout
• Write tests for retry
• Try to get 100% code coverage
• Any instance of polyfill.io need to have proper listing of polyfills (e.g. 38d2252#diff-eacf331f0ffc35d4b482f1d15a887d3bR15 is empty right now)
• Ensure timeout, retry, retryStatusCodes, retryMethods, retryAfterStatusCodes, handleRetry are documented
• There is a queue option I added but it's not being used, so either remove that or add support for queue (e.g. via https://github.com/sindresorhus/p-queue)