Comments (14)
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.
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.
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.
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.
I'm facing a similar issue
from async-retry.
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.
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.
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.
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.
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.
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.
Bumping this, we would love to have this thing fixed
from async-retry.
+1 :(
from async-retry.
@agalatan any chance of you opening a PR? I really would love to have this thing fixed :D
from async-retry.
Related Issues (20)
- Set dynamic retry timeout in onRetry
- (0 , async_retry_1.retry) is not a function HOT 2
- Make sure `randomize` defaults to true
- Add option to decide if retry should be attempted? HOT 3
- How to retry if response data is empty HOT 1
- NPM Version Outdated HOT 2
- Does the example code work? HOT 3
- Throw previous error if there's a timeout HOT 2
- TypeError error e is not a function and TypeError error Function expected
- Add ability to override the delay for a particular failed invocation
- Feature: set a timeout for the entire retry operation
- Package size increase after async-retry HOT 1
- throw Error inside onRetry
- Why isnβt it using vercel retry as its base?
- Jest test fails without retry
- The documentation for onRetry is wrong HOT 1
- Use of `eslintConfig` leaks out, affecting downstream projects HOT 1
- Passing function as argument does not work HOT 1
- Option has no retries HOT 4
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
π Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
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.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google β€οΈ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from async-retry.