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

fix(neon): Fix undefined behavior in channel callbacks when there is a panic #808

Merged
merged 1 commit into from
Oct 4, 2021

Conversation

kjvalencik
Copy link
Member

@kjvalencik kjvalencik commented Sep 30, 2021

Summary

  • Catches panics and converts to uncaughtException
  • Throws uncaughtException if throwing
  • Passes opaque exceptions through as JsBox

Significant Changes

  • Return Err(Throw) in Channel::send no longer disappears. Instead it will cause an uncaughtException
  • Panicking in Channel::send is no longer undefined behavior. Instead it will cause an uncaughtException

What

uncaughtException in pseudo code look like:

const error = new Error("General neon message describing that there was an exception and/or panic");

// Exception was thrown while executing
if (exception) {
    error.cause = exception;
}

// Panic occurred while executing
if (panic) {
    // We were able to downcast the panic as a `String` or `&str`
    if (panicMsg) {
        error.panic = new Error(panicMsg);
    } else {
        error.panic = new Error("Unknown panic");
        error.panic.cause = JsBox(panic);
    }
}
interface UncaughtException extends Error {
    message: string;
    
    cause?: Error;

    panic?: Error & {
        cause?: JsBox
    };
}

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 is super clean and well-thought-out. 👨‍🍳 💋

I made a couple minor suggestions for clarity, but I don't think it needs a re-review unless you want it.

test/napi/src/js/threads.rs Show resolved Hide resolved
test/napi/src/js/threads.rs Outdated Show resolved Hide resolved
test/napi/lib/threads.js Outdated Show resolved Hide resolved
crates/neon-runtime/src/napi/error.rs Show resolved Hide resolved
crates/neon-runtime/src/napi/tsfn.rs Show resolved Hide resolved
…a panic

* Catches panics and converts to uncaughtException
* Throws uncaughtException if throwing
* Passes opaque exceptions through as JsBox
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.

None yet

2 participants