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

Reference Leak for async class functions until owning class instance is released on Node <= 16 #2104

Open
raydin opened this issue May 8, 2024 · 1 comment

Comments

@raydin
Copy link

raydin commented May 8, 2024

When you have a async method on a class like the following nop method

#[derive(Debug, Clone)]
#[napi]
pub struct Example {
    pub name: String,
}

#[napi]
impl Example {
    #[napi(constructor)]
    pub fn new(name: String) -> Self {
        Self {
            name
        }
    }

    #[napi]
    pub async fn nop(&self, result: bool) -> napi::Result<bool> {
        Ok(result)
    }
}

Each call to nop will leave a leaked v8impl::Reference until the instance of Example is released. I think is caused by the issue mentioned here nodejs/node#39915 so not actually a NAPI-RS problem.

Node: v16.15.1
napi-rs: 2.16.6

The example loop that shows this is as follows

import { Example } from '../../core'

async function run() {
    var counter = 0

    let status = () => {
        console.log("Iterations: " + counter.toLocaleString('en-us'))
        let info = Object.entries(process.memoryUsage())
        for (const [key, value] of info) {
            console.log(`${key}: ${value / 1000000} MB `)
        }
        process.stdout.cursorTo(0);
        process.stdout.moveCursor(0, -(info.length + 1));
    }

    var run = true;
    var outputTime = Date.now();

    setTimeout(function () {
        run = false;
    }, 5 * 60000);

    let example = new Example("verify")

    var outputTime = Date.now();
    while (run) {
        let currentTime = Date.now();

        await example.nop(true)
        counter++
        if (currentTime - outputTime > 5000) {
            // If this line is uncommented the leak will not happen
            // example = new Example("verify")
            if (global.gc) {
                global.gc();
            }
            status()
            outputTime = currentTime;
        }
    }
}

run()

The explicit GC calls are just to make it easier to see and not required.

For our use case I can create a version without the issue since we do not actually need to hold onto the reference as our struct can be cloned before going across the async boundary. This will require using less of the auto-genrated code however. Just wanted to verify that this is caused by the current napi limitations before committing to the workaround.

Is it worth changing the expansion code to use clone if the struct supports rather then using napi references for self?

@raydin
Copy link
Author

raydin commented May 16, 2024

Looking at this a little deeper is seems that the issue is resolve in later Node versions. I think it was resolved in Node 18 when TrackedFinalizer was added to its Reference management.

Not verified that this is the cause but specifically older node leaves unlinking and deletion to finalization if the refcount is 0.

The result is in Node 16 and likely older versions is using napi on an async fn will result in increasing RSS usage for every call until the class instance is finalized.

Ideally we would be using a single Reference for every call while incrementing RC, wrapping the a node Reference to self rather then just the pointer to self would allow this but might trip up some current users who are manually unwrapping. Could also use the internal napi-rs REFERENCE_MAP to get a single instance on call but that is not ideal from a performance point of view.

The option I am drawn to is allowing the user to enable wrapping a Reference for this by adding options to the macro and documenting the limitation. Or just avoid old node versions.

@raydin raydin changed the title Reference Leak for async class functions until owning class instance is released Reference Leak for async class functions until owning class instance is released on Node <= 16 May 17, 2024
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