Skip to content

Conversation

@Gentle
Copy link

@Gentle Gentle commented Jan 27, 2025

this enables users to write a custom onerror handler for uncaught rejected promises

the parameters are currently the same as the C API has, this function receives both handled and unhandled rejections.

For the purpose of onerror, you would ignore all calls where is_handled is true as below

example usage:

use rquickjs::{Context, Runtime};

fn main() {
    let runtime = Runtime::new().unwrap();
    let context = Context::full(&runtime).unwrap();
    runtime.set_host_promise_rejection_tracker(Some(Box::new(
        |_ctx, _promise, reason, is_handled| {
            if !is_handled {
                dbg!(reason);
            }
        },
    )));
    context
        .with(|ctx| {
            let _: () = ctx
                .eval("function x() { new Promise(() => { throw new Error('42') }) }; x()")
                .unwrap();
        });
}

@Gentle
Copy link
Author

Gentle commented Jan 27, 2025

where would I put a test for this?

@Gentle
Copy link
Author

Gentle commented Jan 27, 2025

ok, while this works, it causes ref_count errors, I got these logs when debugging

Assertion failed: p->ref_count == 0 (/Users/gentle/work/quantum/quantum-native/target/wasm32-wasip1/wasm-release/build/rquickjs-sys-c03be711d9001ed4/out/quickjs.c
free_zero_refcount: 5617)

I'll try debugging further tomorrow, do you have any insights on what needs to be changed?

@Gentle
Copy link
Author

Gentle commented Jan 27, 2025

I think the refcount error happens because I capture Ctx in a closure here but I'm still trying to figure out how to do that differently

@Gentle
Copy link
Author

Gentle commented Jan 27, 2025

the current iteration is sometimes causing the garbage collector to crash when shutting down, but otherwise does the job

@Gentle
Copy link
Author

Gentle commented Jan 29, 2025

I'm happy to say I fixed it, I'll write a test tomorrow

Copy link
Owner

@DelSkayn DelSkayn left a comment

Choose a reason for hiding this comment

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

Looks good, just need a change to the test to make sure it runs when parallel is enabled.

Copy link
Owner

@DelSkayn DelSkayn left a comment

Choose a reason for hiding this comment

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

LGTM

@DelSkayn DelSkayn merged commit dda9c73 into DelSkayn:master Jan 29, 2025
29 checks passed
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