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

Exception in function shouldRetryRequest() when checking HTTP methods to retry #1

Closed
alex-polunochev-zz opened this issue Feb 28, 2018 · 16 comments · Fixed by #83
Closed
Labels
bug Something isn't working released

Comments

@alex-polunochev-zz
Copy link

alex-polunochev-zz commented Feb 28, 2018

In function shouldRetryRequest there's this block of code

    // Only retry with configured HttpMethods.
    if (!err.config.method ||
        config.httpMethodsToRetry.indexOf(err.config.method.toUpperCase()) < 0) {
        return false;
    }

that fails evaluating second condition, because httpMethodsToRetry is an object, not an array.

Below goes the output from dev console, when the breakpoint was set on the line given above ^^^:

config.httpMethodsToRetry
{0: "GET", 1: "HEAD", 2: "OPTIONS", 3: "DELETE", 4: "PUT"}

That looks different from what gets set in raxConfig

    const raxConfig = {
    ....
        
      // HTTP methods to automatically retry.  Defaults to:
      // ['GET', 'HEAD', 'OPTIONS', 'DELETE', 'PUT']
      httpMethodsToRetry: ['GET', 'HEAD', 'OPTIONS', 'DELETE', 'PUT'],
    }
@JustinBeckwith
Copy link
Owner

Weird. httpMethodsToRetry should be an array, not an object. It would be super helpful if you could share your code :)

@RobertFischer
Copy link

RobertFischer commented Apr 5, 2018

I am getting this, too.

Here's my relevant code.

import Axios from 'axios';
import { attach as raxAttach, getConfig as raxConfig } from 'retry-axios';

const createAxios = () => {
  const http = Axios.create();
  http.defaults.timeout = 60*1000;
  http.defaults.validateStatus = (status) => (status >= 200 && status < 300) || (status == 404);
  http.defaults.raxConfig = {
    instance: http,
    retry: 5,
    noResponseRetries: 100,
    retryDelay: 250,
    httpStatusCodesToRetry: [[100, 199], [420, 429], [500, 599]],
    onRetryAttempt: (err) => {
      try {
        console.info("Retrying request", err, raxConfig(err))
      } catch(e) {
        throw new Error("Error logging the retry of a request: " + e);
      }
    },
  };
  raxAttach(http);
  return http;
};

@chaitya1001
Copy link

I am also getting error,

Here is my relevant code,

axios.defaults.raxConfig= {
  // Retry 3 times on requests that return a response (500, etc) before giving up.  Defaults to 3.
  retry: 3,

  // Retry twice on errors that don't return a response (ENOTFOUND, ETIMEDOUT, etc).
  noResponseRetries: 3,

  // Milliseconds to delay at first.  Defaults to 100.
  retryDelay: 0,

  // HTTP methods to automatically retry.  Defaults to:
  // ['GET', 'HEAD', 'OPTIONS', 'DELETE', 'PUT']
  httpMethodsToRetry: ['GET', 'DELETE', 'PUT', 'POST'],

  // The response status codes to retry.  Supports a double
  // array with a list of ranges.  Defaults to:
  // [[100, 199], [429, 429], [500, 599]]
  httpStatusCodesToRetry: [[100, 199], [429, 429], [500, 599]],

  // If you are using a non static instance of Axios you need
  // to pass that instance here (const ax = axios.create())
  instance: axios,

  // You can detect when a retry is happening, and figure out how many
  // retry attempts have been made
  onRetryAttempt: (err) => {
    const cfg = rax.getConfig(err);
    console.log(`Retry attempt #${cfg.currentRetryAttempt}`);
  }
};
const id = rax.attach(axios);

And here is the error,

TypeError: config.httpMethodsToRetry.indexOf is not a function
    at shouldRetryRequest (/Users/chaityashah/FracTEL/skrum/node_modules/retry-axios/build/src/index.js:96:35)
    at onError (/Users/chaityashah/FracTEL/skrum/node_modules/retry-axios/build/src/index.js:58:10)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)

@luke3butler
Copy link

luke3butler commented Jul 31, 2018

I debugged this issue and found the root of the problem in the Axios package.

The issue only happens on retry, and the reason is because of a configuration merge function that converts the httpMethodsToRetry and statusCodesToRetry arrays to objects.

Offending line here: https://github.com/axios/axios/blob/ad1195f0702381a77b4f2863aad6ddb1002ffd51/lib/core/Axios.js#L35

It appears that this merge function has entirely changed (with deep-copy fixes) for v0.18.x releases, which are yet to be released.

@jordanfloyd
Copy link

also had this issue. An effective workaround for the moment is to pass a custom shouldRetry function and check the type of config.httpMethodsToRetry and get the values into something useful before checking. Not ideal, but relatively easy to fix until a new version of axios is released which resolves the issue.

@JustinBeckwith JustinBeckwith added the bug Something isn't working label Nov 26, 2018
@mpyw
Copy link

mpyw commented Feb 7, 2019

retry-axios seems be incompatible with the latest axios. It doesn't accept custom config raxConfig anymore.

https://github.com/axios/axios/blob/4f98acc57860721c639f94f5772138b2af273301/lib/core/Axios.js#L37

config = mergeConfig(this.defaults, config);

https://github.com/axios/axios/blob/4f98acc57860721c639f94f5772138b2af273301/lib/core/mergeConfig.js#L13-L51

module.exports = function mergeConfig(config1, config2) {
  // eslint-disable-next-line no-param-reassign
  config2 = config2 || {};
  var config = {};

  utils.forEach(['url', 'method', 'params', 'data'], function valueFromConfig2(prop) {
    if (typeof config2[prop] !== 'undefined') {
      config[prop] = config2[prop];
    }
  });

  utils.forEach(['headers', 'auth', 'proxy'], function mergeDeepProperties(prop) {
    if (utils.isObject(config2[prop])) {
      config[prop] = utils.deepMerge(config1[prop], config2[prop]);
    } else if (typeof config2[prop] !== 'undefined') {
      config[prop] = config2[prop];
    } else if (utils.isObject(config1[prop])) {
      config[prop] = utils.deepMerge(config1[prop]);
    } else if (typeof config1[prop] !== 'undefined') {
      config[prop] = config1[prop];
    }
  });

  utils.forEach([
    'baseURL', 'transformRequest', 'transformResponse', 'paramsSerializer',
    'timeout', 'withCredentials', 'adapter', 'responseType', 'xsrfCookieName',
    'xsrfHeaderName', 'onUploadProgress', 'onDownloadProgress', 'maxContentLength',
    'validateStatus', 'maxRedirects', 'httpAgent', 'httpsAgent', 'cancelToken',
    'socketPath'
  ], function defaultToConfig2(prop) {
    if (typeof config2[prop] !== 'undefined') {
      config[prop] = config2[prop];
    } else if (typeof config1[prop] !== 'undefined') {
      config[prop] = config1[prop];
    }
  });

  return config;
};

opyate added a commit to opyate/retry-axios that referenced this issue Feb 14, 2019
opyate added a commit to opyate/retry-axios that referenced this issue Feb 14, 2019
@gpetrioli
Copy link

@opyate The same problem exists with the httpMethodsToRetry config property.
Could you please patch that as well, since it is the same problem ?

opyate added a commit to opyate/retry-axios that referenced this issue Feb 14, 2019
opyate added a commit to opyate/retry-axios that referenced this issue Feb 14, 2019
@georgyfarniev
Copy link

Having this problem as well

@mpyw
Copy link

mpyw commented Feb 15, 2019

@georgyfarniev I abondoned retry-axios and decided to wrap axios by myself. Currently this library is completely imcompatible with the latest axios.

@mpyw
Copy link

mpyw commented Feb 15, 2019

I also propose that peer dependency be correctly defined with a specific version.

  "peerDependencies": {
    "axios": "*" // <- ??
  },

@JustinBeckwith
Copy link
Owner

Greetings! When you say the latest version - what specific version are you using?

@mpyw
Copy link

mpyw commented Feb 15, 2019

@JustinBeckwith

v0.19.0-beta.1. Please see my comment.
Now options are completely whitelisted and it doesn't accept raxConfig.

@opyate
Copy link

opyate commented Feb 15, 2019

@mpyw shall we create separate issues for the peer dependency and whitelisting stuff?

@avindra
Copy link

avindra commented May 22, 2019

Do you guys know of a drop in (or as close to drop in as possible) replacement for axios?

This library is on the critical path for a lot of our apps, and problems like these cause serious issues and slow down development.

@JustinBeckwith
Copy link
Owner

I wrote https://github.com/googleapis/gaxios/ to be that drop in replacement based on node-fetch 🙃 It's part of the reason I don't spend a ton of time in this particular module. It has retries baked in.

@JustinBeckwith
Copy link
Owner

🎉 This issue has been resolved in version 2.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging a pull request may close this issue.