Skip to content

Conversation

apendua
Copy link
Contributor

@apendua apendua commented Oct 22, 2022

Fixes #45400

I wasn't entirely sure where I should be adding unit tests for a fix like this one so would really appreciate some guidance in that area.

Regarding the fix itself, let me try to explain why I believe the previous code was incorrect. In the following snippet:

var __asyncDelegator =
  (this && this.__asyncDelegator) ||
  function (o) {
    var i, p;
    return (
      (i = {}),
      verb('next'),
      verb('throw', function (e) {
        throw e;
      }),
      verb('return'),
      (i[Symbol.iterator] = function () {
        return this;
      }),
      i
    );
    function verb(n, f) {
      i[n] = o[n]
        ? function (v) {
            return (p = !p)
              ? { value: __await(o[n](v)), done: n === "return" }
              : f
              ? f(v)
              : v;
          }
        : f;
    }
  };

the iterator returned by __asyncDelegator reports done = true one step too early when iterator.return() is called. Instead it should emit { value: __await(o[n](v)), done: false } and wait until the correlated __asyncGenerator will call iterator.next(...) with the result of the promise wrapped with __await, say result = { value, done }, obtained from the inner generator via o.return(v). Since iterator is alternating it's behavior on each call (because of p = !p), iterator.next will return { value, done } explicitly this time and if result.done happens to be true, then the processing will stop as expected. On the other hand, if result.done = false, e.g. when there is a yield in finally block, the processing will continue until done = true is eventually observed.

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Oct 22, 2022
@apendua apendua changed the title Fix asyncDelegator reporting done too early Fix asyncDelegator reporting "done" too early Nov 4, 2022
@sandersn sandersn requested a review from rbuckton November 15, 2022 00:59
@rbuckton
Copy link
Contributor

To test this you could add an evaluation test in src/testRunner/unittests/evaluation/asyncGenerator.ts

@rbuckton
Copy link
Contributor

The __asyncDelegator helper is essentially a tick/tock operation. When the delegated generator is called with next/throw/return, it invokes the underlying generator and wraps the result in a pseudo-await ("tick"). When the await completes, it should then return the iteration result ("tock"). That tick/tock operation should always be paired, but the helper currently short-circuits out because of the n === "return" test and is the wrong behavior.

This looks like the correct fix, so I can approve and merge if you're willing to add an evaluation test. We'll need to ensure the __asyncDelegator helper is fixed in tslib as well.

@apendua
Copy link
Contributor Author

apendua commented Nov 15, 2022

@rbuckton Thank you for looking into this PR and for pointing me to

src/testRunner/unittests/evaluation/asyncGenerator.ts

😄

As requested, added a unit test based on the example from my original issue report and I also created a parallel pull request here:

microsoft/tslib#187

@rbuckton rbuckton merged commit dfc1242 into microsoft:main Nov 15, 2022
@apendua
Copy link
Contributor Author

apendua commented Nov 16, 2022

@rbuckton Thank you for merging this! This one is of a particular importance for my team at the moment, so it's much much appreciated 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Yields are ignored in finally block when async generator delegation is used
3 participants