Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

onFailedAttempt returns wrong attempt counts #11

Closed
theverything opened this issue Oct 23, 2018 · 1 comment
Closed

onFailedAttempt returns wrong attempt counts #11

theverything opened this issue Oct 23, 2018 · 1 comment

Comments

@theverything
Copy link
Contributor

The error object returned in onFailedAttempt returns the wrong counts for the attempts.

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

function test(times) {
  let c = 0;
  return function prom() {
    let p;

    if (times === c) {
      p = Promise.resolve('true');
    } else {
      p = Promise.reject(new Error('false'));
    }

    c += 1;

    return p;
  };
}

const f = test(3);

retry(f, {
  retries: 3,
  maxTimeout: 0,
  minTimeout: 0,
  onFailedAttempt(e) {
    console.log(`Attempt ${e.attemptNumber} failed. There are ${e.attemptsLeft} attempts left.`);
  },
})
  .then(v => {
    console.log('success', v);
  })
  .catch(e => {
    console.log('fail', e);
  });

The above prints out

$ node index.js
Attempt 1 failed. There are 2 attempts left.
Attempt 2 failed. There are 1 attempts left.
Attempt 3 failed. There are 0 attempts left.
success true

Maybe I'm using it wrong but if I have some type of logging in onFailedAttempt that checks to see if error.attemptsLeft === 0 and then logs that an operation failed. That may not be true. The very last attempt could still succeed.

@acostalima
Copy link
Contributor

acostalima commented Nov 3, 2018

Yes, you are right. Thanks for raising the issue and sorry for the late reply.

I believe the issue was introduced as a result of an oversight when reading through retry's documentation which states "when retries is set to 1 it means do it once and retry once". On top of the changes you propose in #12, and for the sake of correctness, maybe we should rename attemptsLeft to retriesLeft because, apparently, the first attempt does not account for a retry. This change obviously introduces a breaking change, but I think it's worth it.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants