`retry` is not working as expected

[eloiqs]

Hello,

I think the retry function exported from the rtk query package has not been working as expected for a while. I was able to trace back to a specific commit

To reproduce

• wrap base query with retry
• chrome network tab, throttle option set to "offline"
• trigger a query in the app by clicking a button or something

Expected

• query throws something like TypeError: network request failed
• query is retried 5 times (default maxRetries value)
• after 5 unsuccessful retries, no more requests are made and my code receives the TypeError

Actual

• requests are retried indefinitely

[EskiMojo14]

This change looks like it stops supporting throwing the error once the max retries have been reached in the case that the error thrown is not of type HandledError, which kind of means pretty much every errors thrown by a back-end, or a graphql client unless we catch everything in base query and transform into HandledError. I can confirm that maxRetries have no effect whatsoever in my project, it keeps retrying indefinitely. Weird that no one has reported this however?

To respond to your comment on the commit - your base query shouldn't ever throw. Errors should be caught and turned into a { error } object that's returned. Therefore any error returned by your base query would correctly be wrapped in a HandledError.

[eloiqs]

Ok, thanks, I will try it and report back. We do rethrow some errors in our base query.

[eloiqs]

After investigation, changing the throw error call in ou custom base query does make retry behave as expected. However in our case we implemented a graphql base query similar to what is documented in this repo which does throw error. I am referring to those lines. Is this intended?

[phryneas]

@eloiqs RTK Query has the concept of "expected errors" and "unexpected errors" and that example shows both of them - either return { error } or throw error. Generally, a baseQuery shouldn't throw errors at all, and if it does, it's a kind of error that we don't know how to handle at all - so that would straight cancel everything.

[eloiqs]

Yes, I think I understand. That would mean graphql client errors would be expected errors, and any network errors would be unexpected. But what I'm seeing is that the retry wrapper will never stop retrying in the case of network errors. Isn't that odd? It is quite common that production apis might be unreachable due to the nature of a network, and I would expect a retry functionality to solve this problem for us.

[phryneas]

Yeah, that seems like something we should improve in retry - and probably also handle better in the graphqlBaseQuery. PRs in either direction would be very welcome here :)

[stchristian]

While looking into this in the implementation of retry I feel a disturbance in the force here.

Only those errors are considered unknown that have the throwImmediately property but are not instance of HandledError.
Might be better to just check them at the same time: e.throwImmediately && e instanceof HandledError
There is actually two differentiated errors, one is coming from fail and the other is a rethrown error of the baseQuery in the retry loop. Still, I would remove that throw by combining the two conditions because it makes the error handling of baseQuery inconsistent. If some custom baseQuery used this key internally on its error object, the retry would fail and doesn't seem as a documented behaviour.

Secondly the retry condition will only be checked if the error is a HandledError. With this condition removed we could get out of the loop if the maxRetries are reached and potentially resolve the above problem.

[eloiqs]

I've investigated a bit more in the last few days and here are my findings:

fetchBaseQuery returns timeout/fetch/parse errors when fetch itself fails. This is why retry works when blocking the request in the network tab in the case of a fetch query, because those are returned. graphql-request is different in the sense that by default its request method throws ClientError when the api responds with errors object, and it rethrows anything else as unexpected which includes timeout/fetch/parse errors. This means that timeout/fetch/parse errors in the case of a graphql request are treated as unexpected, while they are treated as handled errors in the case of a fetch request.

It looks like it could be possible to change graphqlBaseQuery example so that it returns different error types for the timout/fetch/parse. But it still leaves the question as to why retry wouldn't just stop retrying in any case.

TLDR: I think retry should stop after maxRetries are reached regardless of the type of error thrown

[markerikson]

Out in https://github.com/reduxjs/redux-toolkit/releases/tag/v2.9.2 !