-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
Comments
Thanks for the report. |
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 ? |
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. |
It seems to work fine on 5.7. I added the backport to #11752 |
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
|
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. |
Thank you @gsathya. It's indeed easier to merge. |
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 <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Testcase:
Expected:
Actual:
The text was updated successfully, but these errors were encountered: