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

Delayed retry in README leads to unexpected result #33

Closed
wwahammy opened this issue Jan 29, 2020 · 3 comments
Closed

Delayed retry in README leads to unexpected result #33

wwahammy opened this issue Jan 29, 2020 · 3 comments

Comments

@wwahammy
Copy link

wwahammy commented Jan 29, 2020

In the README.md, the following code exists:

const pRetry = require('p-retry');
const delay = require('delay');

const run = async () => { ... };

(async () => {
	const result = await pRetry(run, {
		onFailedAttempt: async error => {
			console.log('Waiting for 1 second before retrying');
			await delay(1000);
		}
	});
})();

Without knowing the retry module's features, a user may expect that this code would retry the run function every 1 second. This is not accurate as the retry module has an exponential delay function where, by default, it begins with a 1 second delay in between the first two tries and then multiplies 1 second by 2^(number of tries). The code provided then waits a single second before the retry module's delay between tries executes.

Ideally, this example should be removed since it could lead to confusion for new users of the library.

@sindresorhus
Copy link
Owner

Thanks for commenting. That's a good point. I've improved the example now.

@jdmarshall
Copy link

jdmarshall commented Aug 3, 2020

I was just trying to write a test and was surprised to find that my retry was taking almost exactly 1000 ms longer than I expected.

I don't think trying to adding an explicit delay to the onFailedAttempt() is the correct solution here. I think documenting the options pass-through to retry is the correct strategy. For instance:

pRetry(myFunction, { retries: 2, minTimeout: timeout, factor: 1 })

@jdmarshall
Copy link

Additionally, because the retry module is not using arrow functions, it ends up rebinding 'this', which I didn't notice in my initial implementation, and so my delay function was getting undefined as an argument.

The way I'm suggesting avoids that problem, but you might want to work that into any other examples, if for no other reason to signal to people "hey this bit is tricky".

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

3 participants