In try/finally, finally doesnt get called sometimes

help
1

I have following block of code,

  try {
    const parallelLimitSync = Meteor.wrapAsync(parallelLimit);
    parallelLimitSync(calls, limit);
  } catch (error) {
    console.error(error);
    throw new Meteor.Error('failed-backup', 'Global backup failed');
  } finally {
    console.log('Backup complete.');
  }

In above code parallelLimit is async library’s call which executes multiple async functions in parallel.

The issue is sometimes, the finally block doesn’t get hit, i.e log statement, ‘Backup complete’ doesn’t get logged.

Any idea what could be wrong? Thanks.

2

I am not 100% versed in this package but depending on how it handles failures within it they might get thrown as errors that aren’t exceptions. Try, catch, is meant to catch exceptions and not errors. If your code is erring out it will not proceed.

Also since parallelLimitSync async function you may need to use an await…

As a test try and use promise.all() instead of parallelLimitSync to see if it behaves the same. From what I know promise.all() runs async since it just fires all the functions off and waits for them all to return then proceeds.

But on the surface there is nothing that should stop the finally from being fired. From mdn:

The finally -block contains statements to execute after the try -block and catch -block(s) execute, but before the statements following the try...catch...finally -block. Note that the finally -block executes regardless of whether an exception is thrown. Also, if an exception is thrown, the statements in the finally -block execute even if no catch -block handles the exception.`

It’s hard to say without a codepen or something of the function, if you upload a full functioning example I’d be glad to take a real crack at it.

3

The only way I can imagine finally not being called here is if the task never calls it’s callback, which means parallelLimitSync will never yield from a fiber.

We’d need to see which library parallelLimit comes from and what tasks it’s being asked to perform (calls) to see if it never calls the callback

4

Thanks for reply @dcantato and @coagmano.

@coagmano, Iam trying to figure out if tasks performed by parallelLimit as defined by calls array doesn’t call the callback. The tasks in calls array is as follows,

async function (callback) {
      try {
        await instance.storeBackupFile();
      } catch (error) {
        console.log('Error ' + instance.node.managementIp + ' ' + error.message);
      } finally {
        callback();
      }
    };

storeBackupFile is a function which scp’s the file from a network device.

Thus tasks defined in calls array also use finally, so I fail to see when callback will not be called. Any suggestion?

5

Does await instance.storeBackupFile() have any sort of timeout set?
Otherwise that could hang and prevent finally from running, which would prevent the callback being called, which would prevent the outer finally being called

I’m thinking some version of this is happening:

~/scratch via ⬢ v12.14.1
✦ ❯ node
Welcome to Node.js v12.14.1.
Type ".help" for more information.
> hang = new Promise(_=>{})
Promise { <pending> }
> (async () => {try { await hang } finally { console.log("done") } })()
Promise { <pending> }
>

Where finally never runs, because the promise never resolves

6

Thanks @coagmano for pointing out this scenario where finally is not called if promise never resolves. The function storeBackupFile is bit complex. It executes several commands on ssh connection object via await. Not sure if one of the command hangs. Its difficult to reproduce this bug deterministically and so difficult to debug. Thanks for the pointer, though.

7

I would try adding a timeout to storeBackupFile, though that generally means wrapping it in an extra Promise creation function:


function storeBackupFile() {
  return new Promise(async (resolve, reject) => {
    setTimeout(() => reject(new Error("Timeout")), 30 * 1000);
    // Rest of the code the same, except resolving the result instead of returning;
    const result = await doWork();
    resolve(result);
  });
}

Alternatively, you could use a utility funciton to wrap it:

async function callWithTimeout(timeout, task, context, ...args) {
  return new Promise(async (resolve, reject) => {
    setTimeout(() => reject(new Error("Timeout")), timeout);
    resolve(await task.apply(context, args));
  });
}

async function (callback) {
      try {
        await callWithTimeout(30000, instance.storeBackupFile, instance);
        // passing instance as context so `this` still points to the instance when running
      } catch (error) {
        console.log('Error ' + instance.node.managementIp + ' ' + error.message);
      } finally {
        callback();
      }
    };
8

Thanks for the help @coagmano.