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

In async function, returns a value in try block will before await statement in finally block #11960

Closed
zbinlin opened this issue Mar 21, 2017 · 8 comments
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.

Comments

@zbinlin
Copy link
Contributor

zbinlin commented Mar 21, 2017

  • Version: 7.7.3
  • Platform: Arch Linux

Testcase:

async function test() {
    try {
        console.log("1");
        return await Promise.resolve();
    } finally {
        console.log("2");
        await Promise.resolve();
        console.log("3");
    }
}

test().then(() => {
    console.log("4");
});

Expected:

1
2
3
4

Actual:

1
2
4
3
@targos targos added the v8 engine Issues and PRs related to the V8 dependency. label Mar 21, 2017
@targos
Copy link
Member

targos commented Mar 21, 2017

Thanks for the report.
This looks like a V8 bug and it is fixed in Canary. I'll try to bisect.

@targos
Copy link
Member

targos commented Mar 21, 2017

This was fixed in v8/v8@39642fa

/cc @nodejs/v8 can we safely backport this commit to Node 7 (V8 5.5) and/or V8 5.7 ?

@Fishrock123 Fishrock123 added the confirmed-bug Issues with confirmed bugs. label Mar 21, 2017
@bnoordhuis
Copy link
Member

I don't know, you'd have to try. 5.7 should be an easier target than 5.5. If a full back-port is too onerous, you might be able to get away with dropping the ignition (interpreter) changes.

@targos
Copy link
Member

targos commented Mar 21, 2017

It seems to work fine on 5.7. I added the backport to #11752

@targos
Copy link
Member

targos commented Mar 21, 2017

I attempted a backport (without looking in depth at the changes; just applied the patch and fixed conflicts) to v7.x-staging in targos@46b0159. It compiles and the Node.js test suite passes but the OP's testcase only prints

1
2
3

@gsathya
Copy link
Member

gsathya commented Mar 22, 2017

You need the ignition changes for this to work. Can you try merging https://codereview.chromium.org/2672313003/ to 5.5? This is a fix in the parser, instead of ignition. This might be easier to merge.

@targos
Copy link
Member

targos commented Mar 22, 2017

Thank you @gsathya. It's indeed easier to merge. I backported https://codereview.chromium.org/2633353002 with it.
I will do some testing and open a PR tomorrow.

@targos
Copy link
Member

targos commented Mar 23, 2017

#12004

@targos targos mentioned this issue Mar 23, 2017
2 tasks
@targos targos closed this as completed in 07088e6 Mar 25, 2017
targos added a commit to targos/node that referenced this issue Mar 27, 2017
This is a backport of https://codereview.chromium.org/2672313003/. The
patch did not land in V8 because it was superseded by another one but it
is much easier to backport to V8 5.5, was reviewed and passed tests.

Original commit message:

    [async await] Fix async function desugaring

    Previously we rewrote the return statement in an async function from
    `return expr;` to `return %ResolvePromise(.promise, expr),
    .promise`. This can lead to incorrect behavior in the presence of try-finally.

    This patch stores the `expr` of the return statement in a temporary
    variable, resolves and returns the promise at the end of the finally
    block.

    BUG=v8:5896

PR-URL: nodejs#12004
Fixes: nodejs#11960
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

5 participants