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

Best way to handle Fetch errors such as Failed to fetch, NetworkError when attempting to fetch resource, etc. #545

Open
thojanssens opened this issue Nov 17, 2023 · 11 comments

Comments

@thojanssens
Copy link

We have these errors happening only occasionally and I'm not able to reproduce these errors.

  1. Is it possible to retry only when errors are network errors: https://github.com/sindresorhus/is-network-error/blob/main/index.js ?

  2. Does anyone know what causes these errors? Is it normal to explicitly catch and handle these specific errors or is it more likely a problem on my app or server? The server is just a Hetzner VPS with very basic configuration, I can't imagine it being that with the popularity that Hetzner has; as to the app, it only happens very occasionally, the same request will succeed after any subsequent attempt.

@sindresorhus
Copy link
Owner

  1. You could use a hook:
import ky from 'ky';

const response = await ky('https://example.com', {
	hooks: {
		beforeRetry: [
			async ({error}) => {
				if (!isNetworkError(error)) {
					throw error;
				}
			}
		]
	},
	retry: {
		limit: 2, // Number of retry attempts
		methods: ['get'], // Methods to retry
		statusCodes: [0] // Include network errors (status code 0)
	}
});

However, I think it would be useful if Ky had a retry.shouldRetry function. Then it could simply be:

import ky from 'ky';

const response = await ky('https://example.com', {
	retry: {
		shouldRetry: ({error}) => isNetworkError(error)
	}
});
  1. Hard to know for sure. The occasional network errors like 'Failed to fetch' can have various causes, including transient network issues. It's common to handle such errors in your app. The fact that the same request often succeeds on subsequent attempts suggests external factors may be at play, and the basic server configuration may not be the main issue.

@sholladay
Copy link
Collaborator

I think we should wrap fetch errors in a NetworkError class when isNetworkError() returns true, in order to normalize them and make it more obvious what's going on, since the fetch errors are so inconsistent and vague.

We could potentially also update our retry logic to only retry NetworkError and HTTPError, obviating the need for that hook.

However, that does mean that if a new runtime comes along with a different error message unknown to isNetworkError(), then its network errors would not be retried, which is unfortunate.

An alternative approach would be to detect errors that shouldn't be retried, such as type errors caused by malformed options, and then retry everything else.

It just depends on whether you want to err on the side of retries or not, when the error message is unknown. I would prefer retries, myself.

@sindresorhus
Copy link
Owner

I think we should wrap fetch errors in a NetworkError class when isNetworkError() returns true

👍

We could potentially also update our retry logic to only retry NetworkError and HTTPError, obviating the need for that hook.

That sounds like the most pragmatic solution.

However, that does mean that if a new runtime comes along with a different error message unknown to isNetworkError(), then its network errors would not be retried, which is unfortunate.

Unfortunate yes, but the benefits may outweigh this single downside. New runtimes don't come along every day.

@gvillo
Copy link

gvillo commented Oct 1, 2024

wondering if just setting this obj would retry on failed to fetch errors too (plus default behavior):

retry: {
  limit: 2,
  statusCodes: [0, 408, 413, 429, 500, 502, 503, 504],
},

I'm assuming that adding 0 will catch those errors (dunno if it will catch others and possible side effects that I am not seeing ATM) and the omitted props will use defaults, am I right @sindresorhus?

@sholladay
Copy link
Collaborator

@gvillo you shouldn't need that. We currently retry all fetch errors, whether they are network errors or not, unless a response is received with a status code/headers that tell us not to.

What behavior are you seeing exactly?

@gvillo
Copy link

gvillo commented Oct 1, 2024

Basically trying to understand if I have to use is-network-error with ky to stop getting failed to fetch errors, because the readme doesn't say anything avoid it. Sounds great if it retries network errors by default. I'll remove that custom retry config and see how it performs!

Many thanks!

@sholladay
Copy link
Collaborator

Are you trying to make a cross-origin request? IIRC, some browsers say "Failed to fetch" for CORS errors.

@gvillo
Copy link

gvillo commented Oct 2, 2024

ohh! no no, there is no CORS issue at all. I had no issue at all, I was just asking before implementing Ky, because on the docs there is no mention that the library is retrying on network errors automagically. So far so good it's working great today (last night I implemented it). I'll post if something comes up! Thank you so much!

@gvillo
Copy link

gvillo commented Oct 2, 2024

well, we got an exception while hitting our API (just 1 error in Sentry), but the error is an empty object. error is the prop from the catch statement

image

Any ideas?

@sholladay
Copy link
Collaborator

sholladay commented Oct 2, 2024

Unfortunately, your code is written with (very understandable) false assumptions about JavaScript. 😀

const error = new Error('Something is wrong');
console.log(JSON.stringify(error));  // => "{}"

You cannot serialize errors like that because they have no enumerable properties. Personally, I think this ought to be changed as it's a common footgun, but the Node.js/browser teams would probably say it's too late to change.

HAL 9000 meme: I'm sorry Gabriel, I'm afraid I can't do that.

I would recommend using error.toString() instead of JSON.stringify(error). That will give you error.name and error.message, but not error.stack or other error properties. Most of the time, that should be sufficient.

@gvillo
Copy link

gvillo commented Oct 3, 2024

cool thank you @sholladay!, it's a TypeError: Failed to fetch indeed (Instagram browser)

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

4 participants