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 memory leak with callback closures in napi #3706

Open
tronical opened this issue Oct 19, 2023 · 1 comment · May be fixed by #3864
Open

Fix memory leak with callback closures in napi #3706

tronical opened this issue Oct 19, 2023 · 1 comment · May be fixed by #3864
Labels
a:language-javascript JavaScript bindings (mF,bS) bug Something isn't working

Comments

@tronical
Copy link
Member

The following test case should print "deleted" to indicate that we're not leaking memory.

let slint = require("./index");

const registry = new FinalizationRegistry(() => {
    console.log("deleted");
});

let m = slint.loadFile("./test.slint");
let app = new m.App();
app.test.setHandler(() => {
    console.log(app);
});

registry.register(app);

app = null;

global.gc();

Run inside api/napi, alongside a test.slint:

export component App inherits Window {
    callback test();
}

Run with node --expose-gc ./test.js

If you comment out the code in the closure, you'll see that app gets garbage collected.

Our theory is that the reference to the JS closure kept in the Rust implementation is seen as a root by the garbage collector. One approach we discussed as a remedy would be to store all the callback closures inside the JS object (as properties in an array perhaps), instead of using a strong napi::Ref that becomes a root.

@tronical tronical added bug Something isn't working a:language-javascript JavaScript bindings (mF,bS) labels Oct 19, 2023
@tronical
Copy link
Member Author

tronical commented Nov 6, 2023

The theory was wrong in the sense that the GC doesn't collect app yet. Here's an updated test case that "should" be equivalent and that passes:

let slint = require("./index");

async function blah() {
    const registry = new FinalizationRegistry(() => {
        console.log("deleted");
    });

    let m = slint.loadFile("./test.slint");
    let app = new m.App();
    app.test = () => {
        console.log(app);
    };

    registry.register(app);

    app = null;

    await new Promise(resolve => setTimeout(resolve, 0));
    global.gc();
}

blah();

tronical added a commit that referenced this issue Nov 6, 2023
@tronical tronical linked a pull request Nov 6, 2023 that will close this issue
tronical added a commit that referenced this issue Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:language-javascript JavaScript bindings (mF,bS) bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant