Comment about "finally" not running in README example

[mvila]

In the README, there is the following comment in the second example:

// IMPORTANT: Do not return from here since the "finally" clause will not run!

Are you sure about that? I did some tests and I have no problems, no matter how we exit the "try" block (return or throw), the "finally" clause is always executed.

[ide]

Ah, thanks for pointing that out. The finally clause will run but it will run before the returned promise completes.

  yield lock.acquireAsync();
  try {
    return (async function() {
      console.log(1);
      await;
      console.log(2);
    })();
  } finally {
    console.log('finally');
    lock.release();
  }

I believe you'll see: 1, finally, 2. I'll correct the docs.

[mvila]

Well, if you return a promise, it's likely that it will be completed later. But I'm not sure to understand why you would like to do that, what is the point to return a promise in your example? With co we don't want to return promises, most of the time we want to yield them to get a sequential execution flow.

[ide]

If you yield a promise instead of returning it then co has to resume the generator one extra time. This only applies if yielding the promise is the very last thing your generator function does.

[mvila]

So, it's only a matter of optimization? In any case, since it breaks the try/catch/finally contract, it's not a good idea. Nobody wants to do that and I think your note in the example brings more confusion than anything else.

[ide]

The point of calling it out in the example is so that people don't make this mistake. If you're used to writing only yield that's fine -- keep doing that and you won't have any issues. I added the comment based on my experience from the async PHP codebase at facebook where these optimizations did make a modest but measurable difference at scale and required a bit of care when working with try-catch.

[mvila]

OK, thanks your answer. :)

[nvirth]

Currently, there are 2 code examples in the ReadMe, one for co and one general example. Currently, the note mentioned above is present in the general code section, it has been moved there from the co section.

Now, the note is invalid there, isn't it? If you return a Promise without awaiting it, then the finally block will run before the catch of course, but that's a general code issue, not a problem with the lib. You have to await Promises.

I would like to use your code wrapped this way:

async function callWithLock<T = any>(lock: AwaitLock, callback: (() => Promise<T>)): Promise<T> {
    await lock.acquireAsync();
    try {
        return await callback();
    } finally {
        lock.release();
    }
}

Please tell me if you know any issue with this approach!

Thank you!

[ide]

The comment still stands. The code you wrote is fine because you are not returning a promise and are following the comment’s guidance.