Git Product home page Git Product logo

Comments (14)

przemyslaw-wlodek avatar przemyslaw-wlodek commented on May 5, 2024 6

I've faced the same issue and for me it feels like an unexpected behaviour.
Unfortunately it was not easy to catch until I've seen application logs that appeared far after the promise execution (bail).

I think that retries should be simply stopped after bail was called.

Pattern provided in README example breaks TypeScript typings:

  if (403 === res.status) {
    // don't retry upon 403
    bail(new Error('Unauthorized'))
    return
  }

which leads to an awful type-casting e.g.

  if (403 === res.status) {
    // don't retry upon 403
    bail(new Error('Unauthorized'))
    return (null as unknown) as PromiseReturnType;
  }

from async-retry.

agalatan avatar agalatan commented on May 5, 2024 2

I think this wrapper should do it. Once bailed, it won't retry.
I can open a PR, if I get first a green-light from the maintainer.

const retry = require('async-retry');

const _retry = (cb, ...opts) => {
  let bailedOut;

  return retry(
    (bail, ...rest) =>
      new Promise((resolve, reject) => {
        cb((error) => {
          bailedOut = true;
          bail(error);
        }, ...rest).then(
          (result) => {
              if (!bailedOut) {
                resolve(result);
              }
          },
          (error) => {
              if (!bailedOut) {
                reject(error);
              }
          }
        );
      }),
    ...opts
  );
};

from async-retry.

rayzhangcl avatar rayzhangcl commented on May 5, 2024 1

just found this issue before pushing my code to prod. any plan to get this fixed? it's been 2 years since the issue was reported.

from async-retry.

agalatan avatar agalatan commented on May 5, 2024 1

Is this actually still an issue? I wrote these Jest tests that pass, which suggest it works just fine:

// @flow strict
import retry from 'async-retry';

describe('async-retry', () => {
  const bailError = new Error('BAIL');
  const rejectError = new Error('BOOM');

  test('retries N times', async () => {
    const cb = jest.fn().mockImplementation(
      async () => {
        throw rejectError;
      }
    );

    await expect(
      retry(cb, { retries: 3, factor: 1, minTimeout: 0 })
    ).rejects.toBe(rejectError);

    expect(cb).toHaveBeenCalledTimes(4);
  });

  test('does not keep retrying once bail out is called', async () => {
    const cb = jest.fn().mockImplementation(async (bail, count) => {
      console.log('START');

      if (count === 2) {
        bail(bailError);
        console.log('BAIL');
      }
      throw rejectError;
    });

    await expect(
      retry(cb, { retries: 5, factor: 1, minTimeout: 0 })
    ).rejects.toBe(bailError);

    expect(cb).toHaveBeenCalledTimes(2);
  });
});

from async-retry.

Abramovick avatar Abramovick commented on May 5, 2024

I'm facing a similar issue

from async-retry.

jamesholcomb avatar jamesholcomb commented on May 5, 2024

I might be running into this as well....would it occur if calling bail() inside a try/catch block within the retrier?

from async-retry.

axnsan12 avatar axnsan12 commented on May 5, 2024

I might be running into this as well....would it occur if calling bail() inside a try/catch block within the retrier?

Yes, if the call to bail() is followed by code that might throw before returning.

from async-retry.

Abramovick avatar Abramovick commented on May 5, 2024

I've done this, and it seems to be working.
Hopefully I'm not adding too much code, but if you're lazy to read it then, just skip to the bottom function where the comment that says "key part for this example" is.

The synopsis of all this is: the try/catch block worked for me.

import retry from 'async-retry';
import merge frorm 'lodash/merge';

const retries = 3;
const defaultOptions = {
  headers: {
    Accept: 'application/json',
    'Content-Type': 'application/json',
  },
  method: 'GET',
};
const isJson = response => {
  const contentType = response.headers.get('Content-Type');
  return !!(contentType && contentType.indexOf('application/json') !== -1);
};

async function request(
  url,
  options = {},
) {
  const allOptions = merge({}, defaultOptions, options);
  const response = await fetch(url, allOptions);

  const json = isJson(response) ? await response.json() : null;

  if (response.ok) {
    return json;
  }

  throw json;
}


/* ------ key part for this example -------  */
const response = await retry(
  async bail => {
    try {
      return await request('https://google.com');

      /* 
       * If the request function definition above is confusing you, just do a normal fetch like this 
       * return await fetch('https://google.com');
       */
    } catch (e) {
      if (e.status >= 400 || e.status < 500) return bail(e);

      throw e;
    }
  },
  {
    retries,
  },
);

from async-retry.

axnsan12 avatar axnsan12 commented on May 5, 2024

Yes, of course that if you do return bail() you avoid this issue.

The actual issue is when you don't do that. More specifically, after calling bail() it should not matter what the outcome of the returned promise is, it should stop retrying even if you do bail() + throw.

from async-retry.

xenjke avatar xenjke commented on May 5, 2024

Were just hit by that as well. Surprising number of unexpected background requests in runtime, while all unit tests were green :) can be caught while unit testing with something like:

// `retryer` being a wrapper that retries `request`, similar to the one above in the comments

it("rejects with passed function error and doesn't make more calls than needed", async () => {
            request.rejects(new Error("custom error"));
            assert.isRejected(retryer(), "custom error"); // actual call to the retrier, pass, rejected
            sinon.assert.callCount(request, 1)) // pass, just one call so far

            // that was added to catch this issue
            return await new Promise(resolve => {
                // wait to see if there any retries in the background
                // even if the call was rejected
                setTimeout(() => {
                    resolve(sinon.assert.callCount(request, 1)); // will fail, as more requests will be done
                }, 500);
            });
        });

from async-retry.

cdimitroulas avatar cdimitroulas commented on May 5, 2024

I agree with @przemyslaw-wlodek - the Typescript typings are not great.

It would be great if bail had the type of (e: Error) => never. This would allow the return type of the retry function to not have a potential null or undefined.

from async-retry.

albertodiazdorado avatar albertodiazdorado commented on May 5, 2024

Bumping this, we would love to have this thing fixed

from async-retry.

glebsts avatar glebsts commented on May 5, 2024

+1 :(

from async-retry.

albertodiazdorado avatar albertodiazdorado commented on May 5, 2024

@agalatan any chance of you opening a PR? I really would love to have this thing fixed :D

from async-retry.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    πŸ–– Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. πŸ“ŠπŸ“ˆπŸŽ‰

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❀️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.