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

Handling of JavaScript errors in ObjectInfo's callback #17

Open
mhofman opened this issue May 10, 2019 · 0 comments
Open

Handling of JavaScript errors in ObjectInfo's callback #17

mhofman opened this issue May 10, 2019 · 0 comments

Comments

@mhofman
Copy link

mhofman commented May 10, 2019

It looks like errors thrown by ObjectInfo's callback cannot be caught in process' "uncaughtException" event.

What's even more surprising is that the error is subsequently thrown the next time an ObjectInfo is constructed.

The workaround is to wrap the callback into a setImmediate call, but that shouldn't be necessary since the native side already does that.

I have a feeling this is an issue for https://github.com/node-ffi-napi/setimmediate-napi

From a quick research I guess calling napi_fatal_exception instead of e.ThrowAsJavaScriptException() would do the trick, but I'm probably out of my depth here.

Repro:

const { ObjectInfo, WeakTag } = require("@mhofman/weak-napi-native");

const map = new WeakMap();

async function test(call) {
    let listener;

    const thrown = new Promise(resolve => {
        listener = error => void resolve(error);
        process.on("uncaughtException", listener);
    });

    let object = {};
    const info = new ObjectInfo(object, () => {
        call(() => {
            throw new Error(
                "I should be able to get this in uncaughtException"
            );
        });
    });

    map.set(object, new WeakTag(info));

    object = undefined;
    await Promise.resolve(resolve => setImmediate(resolve));
    gc();

    const result = await Promise.race([
        thrown,
        new Promise(resolve => setTimeout(resolve, 200, "Bailing")),
    ]);

    process.removeListener("uncaughtException", listener);

    return result;
}

(async () => {
    console.log("setImmediate start");
    console.log(
        "setImmediate",
        await test(fn => {
            console.log("setImmediate callback invoked");
            setImmediate(fn);
        })
    );

    console.log("direct start");
    console.log(
        "direct",
        await test(fn => {
            console.log("direct callback invoked");
            fn();
        })
    );

    try {
        const info = new ObjectInfo({}, () => {});
    } catch (error) {
        console.log("Caught error on construct", error);
    }

    console.log("done");

    process.exit();
})();

Output:

setImmediate start
setImmediate callback invoked
setImmediate Error: I should be able to get this in uncaughtException
direct start
direct callback invoked
direct Bailing
Caught error on construct Error: I should be able to get this in uncaughtException
done
mhofman added a commit to mhofman/tc39-weakrefs-shim that referenced this issue May 10, 2019
Workaround for node-ffi-napi/weak-napi#17
Rewrite node stub to be fully compliant
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

No branches or pull requests

1 participant