softonic / axios-retry

Axios plugin that intercepts failed requests and retries them whenever possible
Other
1.9k stars 167 forks source link

is Axios Network Error properly detected? #138

Open dagi3d opened 4 years ago

dagi3d commented 4 years ago

Hi, I was trying this library to make the app more resilient against possible failed requests because of network errors, and the thing is I am not sure if the default function to detect the network error will work with the current Axios implementation:

when Axios wants to raise the Network Error it sends a null response code:

 request.onerror = function handleError() {
      // Real errors are hidden from us by the browser
      // onerror should only fire if it's a network error
      reject(createError('Network Error', config, null, request));

but in the axios-retry, it will check if there is actually an error code:

export function isNetworkError(error) {
  return (
    !error.response &&
    Boolean(error.code) && // Prevents retrying cancelled requests
    error.code !== 'ECONNABORTED' && // Prevents retrying timed out requests
    isRetryAllowed(error)
  ); // Prevents retrying unsafe errors
}

maybe theres is something I am missing, but is this really working as expected?

subvertallchris commented 4 years ago

I think we're seeing the same thing. The error object that gets into isNetworkError looks like this when we log it out:

Error: Network Error
    at createError (createError.js:16)
    at XMLHttpRequest.handleError (xhr.js:91)

This object is of type AxiosError. I can call error.message and it returns Network Error. A simple workaround for us is to provide a custom check and fall back to the default:

function errorIsAxiosError(error: Error): error is AxiosError {
  return !!(error as any).isAxiosError;
}

function simpleNetworkErrorCheck(error: Error) {
  if (errorIsAxiosError(error) && error.message === 'Network Error') {
    return true;
  } else {
    return isNetworkOrIdempotentRequestError(error);
  }
}

axiosRetry(axios, { retryCondition: simpleNetworkErrorCheck });

I'm unsure of whether all browsers and platforms will give us that generic "Network Error" message, so I'll check more but it would be great if anyone tries this out and wants to report their findings.

Uzlopak commented 4 years ago

I think this is because of isRetryAllowed, where ENETUNREACH is in the denylist

https://github.com/sindresorhus/is-retry-allowed/blob/master/index.js

cbrevik commented 4 years ago

Seems related to https://github.com/axios/axios/issues/2351

I did something similar as above here, but instead check for:

const isNetworkError = (error: AxiosError) =>
  error && // just to make sure
  !error.response && // if there is a response, it reached the server and not a network error
  error.code !== 'ECONNABORTED'; // check that it isn't a timeout

And then my retryCondition is:

const retryCondition = (error: AxiosError) =>
  isNetworkError(error) || // my custom check
  isIdempotentRequestError(error); // retry lib check

It's not a perfect network error check, but I'd prefer that to checking for a string message. Ideally this lib should handle it better?

meshuamam commented 1 year ago

Axios does set an error.code on network errors: https://github.com/axios/axios/blob/786b113a40011faaee95da0b02eaa7de06944d7a/lib/adapters/xhr.js#L154

Uzlopak commented 1 year ago

Well. It got a AxiosError about 14 months ago.