Bump node-addon-api to fix memory leak #2436

[iurisilvio]

Fix memory leak from deferred N-API weak reference callbacks

Fixes #2436

Problem

Since the NAN -> N-API migration (v3.0.0), ObjectWrap destructors (which free cairo surfaces) are deferred to the next SetImmediate instead of running during GC. In server environments where the event loop is always busy, these destructors never fire and memory grows indefinitely.

This was reported upstream in nodejs/node-addon-api#1140 and fixed in nodejs/node#42651 by introducing node_api_nogc_finalize, a finalizer that runs directly during GC for native-only cleanup.

Fix

• Upgrade node-addon-api from 7.x to 8.x (which uses node_api_nogc_finalize for ObjectWrap::FinalizeCallback)
• Add NAPI_EXPERIMENTAL to binding.gyp defines (required to enable NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER in Node's headers)

No code changes, node-addon-api 8.x automatically routes ObjectWrap destructor callbacks through the nogc finalizer path when the flag is set.

I don't fully understand the impact of node-addon-api upgrade or enabling NAPI_EXPERIMENTAL.

Before / After

200 requests decoding 3000x2000 JPEG images into thumbnails:

// node --expose-gc stress.js
const { createCanvas, loadImage } = require("canvas");

let s = 1;
function rand() {
    s = (s + 0x6d2b79f5) | 0;
    let t = Math.imul(s ^ (s >>> 15), 1 | s);
    t = (t + Math.imul(t ^ (t >>> 7), 61 | t)) ^ t;
    return ((t ^ (t >>> 14)) >>> 0) / 4294967296;
}

function genImage(w, h) {
    const c = createCanvas(w, h);
    const ctx = c.getContext("2d");
    for (let i = 0; i < 10; i++) {
        ctx.fillStyle = `rgb(${(rand() * 255) | 0},${(rand() * 255) | 0},${(rand() * 255) | 0})`;
        ctx.fillRect((rand() * w) | 0, (rand() * h) | 0, (rand() * w) / 2, (rand() * h) / 2);
    }
    return c.toBuffer("image/jpeg");
}

async function run() {
    const imgs = [genImage(2000, 1500), genImage(3000, 2000)];
    let peak = 0;
    for (let i = 0; i < 50; i++) {
        const promises = [];
        for (let j = 0; j < 4; j++) {
            promises.push(
                loadImage(imgs[j % 2]).then((img) => {
                    const c = createCanvas(200, Math.round((img.height / img.width) * 200));
                    c.getContext("2d").drawImage(img, 0, 0, c.width, c.height);
                    return c.toBuffer("image/jpeg");
                })
            );
        }
        await Promise.all(promises);
        const rss = process.memoryUsage().rss;
        if (rss > peak) peak = rss;
        if ((i + 1) % 10 === 0)
            console.log(`  batch ${i + 1}: rss=${Math.round(rss / 1024 / 1024)}MB`);
    }
    console.log(`Peak RSS: ${Math.round(peak / 1024 / 1024)}MB`);
}

run();

// Before (node-addon-api 8, no NAPI_EXPERIMENTAL):
  batch 10: rss=784MB
  batch 20: rss=1477MB
  batch 30: rss=2169MB
  batch 40: rss=2862MB
  batch 50: rss=3555MB
Peak RSS: 3555MB

// After (node-addon-api 8 + NAPI_EXPERIMENTAL):
  batch 10: rss=196MB
  batch 20: rss=195MB
  batch 30: rss=196MB
  batch 40: rss=196MB
  batch 50: rss=196MB
Peak RSS: 196MB

[chearon]

Great find! That tracks perfectly with what we discussed in the issue, and it looks like we are good to upgrade since we don't support node 16.

The tests are failing, though.

[iurisilvio]

Thank you for running the tests, I didn't run them local before! Now I ran it on Mac and Linux, reproduced the failures local, I hope my last commit fix all of them. 🙏🏻

[iurisilvio]

Nice, all green now! 💚 Looking forward to have it reviewed/merged/released!

[chearon]

From my reading of node-addon-api, having this member function forces the destructor to get called after gc, though before JS goes back to executing. It makes sense since this couldn't get called if the instance were already destroyed. Here's the relevant bit, cleaned up (source):

// If class overrides the basic finalizer, execute it.
if constexpr (details::HasBasicFinalizer<T>::value) {
  HandleScope scope(env);
  instance->Finalize(Napi::BasicEnv(env));
}

// If class overrides the (extended) finalizer, schedule it.
if constexpr (details::HasExtendedFinalizer<T>::value) {
  // Attach via node_api_post_finalizer. `PostFinalizeCallback` is responsible
  // for deleting the `T* instance`, after calling the user-provided finalizer.
  napi_status status =
      node_api_post_finalizer(env, PostFinalizeCallback, data, nullptr);
  NAPI_FATAL_IF_FAILED(status,
                       "ObjectWrap<T>::FinalizeCallback",
                       "node_api_post_finalizer failed");
}
// If the instance does _not_ override the (extended) finalizer, delete the
// `T* instance` immediately.
else {
  delete instance;
}

And the PostFinalizeCallback looks like this:

T* instance = static_cast<T*>(data);
instance->Finalize(Napi::Env(env));
delete instance;

But it would be better to destroy within gc, which is the whole point of BasicEnv. All day I've been trying to figure out why that causes node to crash (your initial test failures). I know it crashes in AdjustExternalMemory, but it seems to be failing because of something internal to node or node-addon-api.

AdjustExternalMemory takes a BasicEnv, and that env should still be valid, so I'm kind of at a loss for now. I'd like to take some time to understand it, which either leads to a bug fix in node or a corrected understanding on my part. I'm a little more hesitant to use the experimental API now. If we wanted to use it now, just having this method be empty and leaving the AdjustExternalMemory below would be better, since we want that to get called if we destroy from the C++ side.

I also think we should probably refactor the classes to only store BasicEnv to avoid accidentally trying to invoke JS at the wrong times. It's pretty easy to grab the full Napi::Env from info or other context.

[iurisilvio]

I added a test to make sure memory leak doesn't come back later.

I don't know how to proceed from here.

[flohall]

Works like a charm for our server-side rendering. Memory issues we had are gone. We already use a merged version internally at the company. Looking forward for the official merge.

[iurisilvio]

I built binaries with this fix for me here.

With all the recent supply chain attacks, you really should not trust me. 😆 The build is based on this PR, you can easily fork it, inspect all the changes and build it for yourself using the github action workflow.

[iurisilvio]

I deployed it today and looks like it didn't fix my memory leak. 😢

Happy that it worked for others.

[iurisilvio]

For completeness, it doesn't mean the fix is not real, others confirmed it works, but probably I have other leaks. 😆

[flohall]

As it did not seem to fix your issues @iurisilvio, I wanted to add what exactly we have used for our testing, in case someone else needs the info.

6a84247

[iurisilvio]

No worries, I'm tracking other memory leaks in this lib and I think my leak is fixed after them.

• Memory leak in JPEG EXIF rotation: rotate90/rotate270 leak the rotated pixel buffer #2572
• Allocator mismatch in decodeJPEGIntoSurface OOM path: free() called on new[]-allocated buffer #2573
• loadImage() doesn't clear image.src on error, delaying cleanup of partial cairo state #2576
• decodeJPEGIntoSurface leaks data and src on libjpeg longjmp from jpeg_read_scanlines #2577
• canvas_state_t leaks cairo_pattern_t members; fillStyle/strokeStyle are weak refs #2578
• CanvasPattern doesn't retain source Image/Canvas, breaks under prompt finalizers #2579
• loadSVGFromBuffer leaks RsvgHandle when SVG has no intrinsic size #2584
• Three rare error paths in image decoders leak native resources #2586