0

I'm writing a function that will take a promise as an argument and if this promise rejects, the function will retry it x times before giving up. The issue I'm having is that after the promise rejects enough times, I get an exception due to not handling a rejected promise, and I haven't been able to figure out why.

My code is included at the bottom, and I've also made a codesandbox with my code. The logic is as follows:

  1. Call retry with the function, the amount of times to re-attempt and the delay in ms between each re-attempt.
  2. retry function attempts to execute the functionthat is passed to it.
  3. If it succeeds, resolve with the result of the function. If it fails, go to step 1 and decrease the attempts variable by 1.
  4. If the attempts variable is 0, retry rejects.

If retry rejects, it's then expected that the catch block in example() (line 61) is execute, but this never happens. Why does it never happen, and how can the code be changed to change this?

Edit: I've just noticed that the rejection isn't really clear in the codesandbox, but this is what happens when retry rejects:

node:internal/process/promises:279
            triggerUncaughtException(err, true /* fromPromise */);
            ^

[UnhandledPromiseRejection: This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason "rejection".] {
  code: 'ERR_UNHANDLED_REJECTION'
}

The full code, as requested:

const wait = (delay = 5000) =>
  new Promise((resolve) => setTimeout(resolve, delay));

const retry = ({ attempts, delay, fn, maxAttempts = attempts }) => {
  if (maxAttempts !== attempts) {
    /**
     * We want to double the delay on each
     * successive attempt after the first.
     */
    delay = delay * 2;
  }
  return new Promise(async (resolve, reject) => {
    try {
      console.log(1, `attempting ${fn.name}...`);
      const result = await fn();
      resolve(result);
    } catch (e) {
      if (attempts === 0) {
        console.log(2, `${fn.name} failed ${maxAttempts} times, giving up.`);
        reject(e);
        return;
      }
      console.log(
        2,
        `${fn.name} failed, retrying in ${
          delay / 1000
        }s... (${attempts} attempts remaining)`
      );
      await wait(delay);
      console.log(3, "retrying");
      retry({
        attempts: attempts - 1,
        delay: delay,
        fn,
        maxAttempts
      });
    }
  });
};

// An example promise that simply rejects after a second
const fakeReject = () =>
  new Promise((_, reject) => setTimeout(() => reject("rejection"), 1000));

const example = async () => {
  try {
    const result = await retry({
      attempts: 3,
      delay: 2000,
      fn: fakeReject
    });
    /**
     * The following would execute if fakeReject were to resolve instead.
     */
    console.log(result);
  } catch (e) {
    /**
     * I expected this block of code to execute once the fakeReject
     * promise has rejected 3 times, but this is never executed.
     */
    console.log("we tried");
  }
};

example()
  .then((x) => {
    console.log("success", x);
  })
  .catch(console.error);

kougami
  • 696
  • 1
  • 6
  • 14
  • 1
    I think one issue is using a new Promise with an `async` executor - you're probably handling the *rejection* in some unexpected way – Jaromanda X Oct 12 '22 at 03:16
  • oh, and by the way `.catch(console.error());` should be `.catch(console.error);` – Jaromanda X Oct 12 '22 at 03:17
  • 1
    Please [edit] your question to include the code of your `retry` function, [do not just post a link to some external site](https://meta.stackoverflow.com/q/254428)! – Bergi Oct 12 '22 at 03:17
  • @Bergi Posting codesandbox links with code is common practice on Stackoverflow as it can be helpful to others to see and use the code in an environment where it can be freely edited and executed. Nonetheless, I've edited the question to add the code as requested. – kougami Oct 12 '22 at 03:19
  • @JaromandaX well spotted on the catch - could you possibly elaborate on what you mean by "handling the rejection in some unexpected way"? I don't disagree but I'm struggling to think of what that unexpected way could be. – kougami Oct 12 '22 at 03:20
  • 2
    yeah, I've never used an `new Promise(async (resolve, reject) =>` pattern, basically because it should never be needed - slight alteration to your code to remove that anti-pattern, and the code works as you would expect ... I can't tell you WHY yours doesn't work though, as I said, it's an anti-pattern I've never used – Jaromanda X Oct 12 '22 at 03:24
  • also, by the way, you say the attempt failed 3 times in the message, whereas it in fact has failed 4 times at that point – Jaromanda X Oct 12 '22 at 03:30
  • @JaromandaX that should be easily fixed but thanks for pointing it out :) I wasn't aware that using async with Promise like that was an anti-pattern, I'll have to look into that. Could you please advise me on what I'd have to change to remove the anti-pattern in this case? Thanks – kougami Oct 12 '22 at 03:32
  • remove `new Promise` ... make `retry` an `async` function instead ... `return` values that you are `resolve`ing and `throw` errors where you are `reject`ing - I'd post the working code as an answer, except I can't actually answer WHY your code doesn't work – Jaromanda X Oct 12 '22 at 03:35

1 Answers1

1

Never pass an async function as the executor to new Promise! In your case you forgot to resolve the returned promise from the catch block of your code, you only made a recursive call - which constructs a new, independent promise that is ignored and whose rejections are never caught. It would have worked if you had written resolve(retry({…})), but really this is not needed:

async function retry({ attempts, delay, fn, maxAttempts = attempts }) { /*
^^^^^ */
  if (maxAttempts !== attempts) {
    delay = delay * 2;
  }
  try {
    console.log(1, `attempting ${fn.name}...`);
    const result = await fn();
    return result;
//  ^^^^^^
  } catch (e) {
    if (attempts === 0) {
      console.log(2, `${fn.name} failed ${maxAttempts} times, giving up.`);
      throw e;
//    ^^^^^
    }
    console.log(2, `${fn.name} failed, retrying in ${delay / 1000}s... (${attempts} attempts remaining)`);
    await wait(delay);
    console.log(3, "retrying");
    return retry({
//  ^^^^^^
      attempts: attempts - 1,
      delay: delay,
      fn,
      maxAttempts
    });
  }
}
Bergi
  • 630,263
  • 148
  • 957
  • 1,375
  • Thank you so much! Could you please clarify how it is that it's necessary to return retry({...}) in order for the function to execute again? Thanks for the link as well, I'll have to read up on that antipattern as I've been doing it a lot. – kougami Oct 12 '22 at 04:28
  • Well it's not strictly necessary to make it execute again, but it's obviously necessary to return the `result` of the recursive call back to the caller. – Bergi Oct 12 '22 at 04:31