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

'operation.attempt' callback gets executed more than once with same 'attempt' number #45

Closed
rightaway opened this issue Jan 20, 2017 · 7 comments

Comments

@rightaway
Copy link

I've tried to make node-retry work with Promises, so that I can call it for example like await retryCommand('curl http://example.com'). But it's not working as expected, because I get numerous console.log output with the same attempt number before the attempt number gets incremented. For example:

doing 2: curl http://example.com
doing 2: curl http://example.com
doing 2: curl http://example.com
...
doing 3: curl http://example.com
doing 3: curl http://example.com
doing 3: curl http://example.com

Why would the operation.attempt callback get executed again with the same attempt number? I'm passing an async callback to operation.attempt but I don't see why that would be the problem.

function retryCommand(command) {
  return new Promise((resolve, reject) => {
    const operation = retry.operation({})
    operation.attempt(async (attempt) => {
      if (attempt > 1) console.log(`doing ${attempt}: ${command}`)
      try {
        const execResult = await execPromise(command)
        resolve(execResult)
      } catch (e) {
        if (errorIsRetriable(e)) {
          if (operation.retry(e)) return
          reject(operation.mainError())
        } else {
          reject(e)
        }
      }
    })
  })
}

Basically the code checks if the error should be retried (with a custom errorIsRetriable(e) function), and if not then the Promise rejects right away.

@matomesc
Copy link

matomesc commented Mar 6, 2017

Not sure why this happens, however I've opted to use the following approach until we can figure this issue out:

const maxAttempts = 3;
let attempt = 1;
while (attempt <= maxAttempts) {
  try {
    console.log(`doing ${attempt}: ${command}`);
    const execResult = await execPromise(command);
    // Do something with execResult
    cosole.log(execResult)
    break;
  } catch (e) {
    this.log.error(e);
    // Sleep using linear backoff strategy (5s after first attempt, 10s after second attempt)
    if (attempt < maxAttempt) {
      await new Promise(resolve => setTimeout(resolve, 5000 * attempt));
    }
    attempt += 1;
  }
}

@rightaway
Copy link
Author

So you are getting the same issue as me? Do you also get it with node-promise-retry?
See my issue in IndigoUnited/node-promise-retry#14, I'm getting it there too.

@matomesc
Copy link

matomesc commented Apr 6, 2017

@rightaway Yep same issue. I don't use node-promise-retry but it's just a wrapper around node-retry so it makes sense that you encountered this issue there as well.

@tim-kos
Copy link
Owner

tim-kos commented Apr 6, 2017

Hey ladies and gentlemen,

I will look into this soon. I am just really busy with Transloadit right now.

Sorry for the delay. :/

@avranju
Copy link

avranju commented Nov 20, 2017

FWIW, the following test program seems to work fine for me.

const retry = require("retry");

const sleep = timeout =>
    new Promise(resolve => setTimeout(resolve, timeout));

function log(msg) {
    console.log(`${new Date().toLocaleString()}: ${msg}`);
}

async function asyncThrow() {
    await sleep(1000);
    throw 'whoops';
}

const operation = retry.operation({
    retries: 3,
    factor: 2,
    maxTimeout: 10000,
    randomize: true
});
operation.attempt(async (attemptNumber) => {
    try {
        log(`attemptNumber = ${attemptNumber} - calling asyncThrow`);
        await asyncThrow();
    }
    catch(err) {
        if (operation.retry(err)) {
            log('retrying.');
        } else {
            log('giving up.');
        }
    }
});

Here's the output I get:

2017-11-20 14:34:00: attemptNumber = 1 - calling asyncThrow
2017-11-20 14:34:01: retrying.
2017-11-20 14:34:02: attemptNumber = 2 - calling asyncThrow
2017-11-20 14:34:03: retrying.
2017-11-20 14:34:07: attemptNumber = 3 - calling asyncThrow
2017-11-20 14:34:08: retrying.
2017-11-20 14:34:13: attemptNumber = 4 - calling asyncThrow
2017-11-20 14:34:14: giving up.

@tim-kos
Copy link
Owner

tim-kos commented Dec 11, 2017

Hm okay, thanks for doing this.

@rightaway Do you mind posting your complete code here?

@tim-kos
Copy link
Owner

tim-kos commented Mar 28, 2018

Closing this then for now, because the error is unlikely in node-retry, but rather in the userland code. @avranju 's example works as expected.

@tim-kos tim-kos closed this as completed Mar 28, 2018
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

4 participants