Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retry middleware not retrying #58

Closed
danny-larsen opened this issue May 10, 2018 · 8 comments
Closed

Retry middleware not retrying #58

danny-larsen opened this issue May 10, 2018 · 8 comments

Comments

@danny-larsen
Copy link
Contributor

danny-larsen commented May 10, 2018

I cannot get the retry middleware working.

I am trying to get it to retry on fetch failures, but that does not seem to work. Is it only working for requests that actually return a status code?

EDIT
So after poking around in the code I see that it is only retrying for timeout errors. Is there a reason for that?

if (err === timeoutError) {
var retryDelayMS = retryAfterMs(attempt);
if (retryDelayMS) {
logger('response timeout, retrying after ' + retryDelayMS + ' ms');
return sendTimedRequest(timeout, retryDelayMS);
}
}

@nodkz
Copy link
Collaborator

nodkz commented May 10, 2018

You should take a look at another line, which also makes retry by code:
https://github.com/nodkz/react-relay-network-layer/blob/3af7e69f9eb83b246d6d65a31e5935cdfdabecd2/src/middleware/retry.js#L64

It works for any non 2xx codes. Status codes returned by fetch method.

Can you provide your fetch response or error?

It's very interesting what kind of error did you get. If it will be reasonable we can improve this middleware.

@danny-larsen
Copy link
Contributor Author

I am not quite sure. I get some errors stating "TypeError: Failed to fetch" or just "Failed to fetch". I tried to simulate it by setting the browser to "offline" mode and it did not seem to do the retries.

@nodkz
Copy link
Collaborator

nodkz commented May 22, 2018

Do you use Relay Classic or Relay Modern?
This package react-relay-network-layer for Relay Classic
react-relay-network-modern for Relay Modern

For now I do not have any Classic app, and honestly time for debugging this issue. But when we use classic relay it works perfectly in the browser. Please try to debug it yourself:

@danny-larsen
Copy link
Contributor Author

I am using relay classic. I have tested a little and when the request to the graphql server fails, that promise fails and then it is only retrying when it is a timeout error.
This means that the line 64 referred to above is never reached. It is in the catch in line 70 and I just don't get why the "if (err === timeoutError)" is there?
If I remove that if statement, then it is retrying, also when errors happen.

@nodkz
Copy link
Collaborator

nodkz commented May 22, 2018

Errors may be different eg 403, 404 and we should them pass to other middlewares.
So you need to catch that error exactly Failed to fetch and add it to line 71 via or operator.

@danny-larsen
Copy link
Contributor Author

After some debugging, I found out that Fetch will only reject the promise when an error happened during the fetching. It does not reject when getting 4xx or 5xx HTTP error codes or any other HTTP error codes so these must be handled inside the "then", which they also seem to be in line 60.

I tried looking into catching the error in the catch, but it seems that different browsers will return different error messages on network errors so it is quite hard to catch them all. Chrome returns "Failed to fetch", Firefox returns "NetworkError when attempting to fetch resource." and Safari returns "The network connection was lost.".

So I am still unsure if it is even possible to end up in the catch in line 70 in any other cases than the timeout or a fetch error.

@nodkz
Copy link
Collaborator

nodkz commented May 22, 2018

After some debugging, I found out that Fetch will only reject the promise when an error happened during the fetching. It does not reject when getting 4xx or 5xx HTTP error codes or any other HTTP error codes so these must be handled inside the "then", which they also seem to be in line 60.

Nice. Let's comment line 70, and publish a new version. I think after such debugging it's quite safe to make such changes. Please open PR.

@danny-larsen
Copy link
Contributor Author

Added pull request #59

@nodkz nodkz closed this as completed in ad5c0db May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants