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

Avoid leaving a dangling future when handling a rejection in JsPromise::to_future #1008

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

kjvalencik
Copy link
Member

@kjvalencik kjvalencik commented Nov 17, 2023

Previously, Neon was creating a distinct promise chain for handling resolved promises in JsPromise::to_future. This is problematic because it causes an unhandledRejection, even though the user may be handling the error.

In other words, it was the equivalent of this JavaScript:

const p = Promise.reject(new Error("oh, no!"));

p.then(() => console.log("Everything is good!"));
p.catch(() => console.log("Handled the error"));
Handled the error
/private/tmp/try-catch/foo.js:1
const p = Promise.reject(new Error("oh, no!"));
                         ^

Error: oh, no!

Instead, we use the two argument version of .then(onResolved, onRejected) which accepts both handlers at once, acting sort of like a match result. It is equivalent to the following JavaScript:

let settled = false;

Promise
    .reject(new Error("oh, no!"))
    .then(() => {
        if (!settled) {
            console.log("Everything is good!")
        }
    }, () => {
        settled = true;

        console.log("Handled the error")
    });

@kjvalencik
Copy link
Member Author

kjvalencik commented Nov 20, 2023

.then can take a rejection handler as a second argument. This is a somewhat simpler option.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then#onrejected

Ideally, we also wouldn't need to create new JsFunction wrapping Rust on each to_future, but the only alternative I can think of is .bind(..).

Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a good fix, but I like your idea of using the two-argument variant of .then() better.

crates/neon/src/handle/mod.rs Show resolved Hide resolved
crates/neon/src/types_impl/boxed.rs Show resolved Hide resolved
Copy link
Collaborator

@dherman dherman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@kjvalencik kjvalencik merged commit a6b4d13 into main Nov 27, 2023
9 checks passed
@kjvalencik kjvalencik deleted the kv/to-future-fix branch November 27, 2023 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants