Memory leak. ArrayBuffer finalizeCallback never called

[AlexVdberg]

I'm running into massive memory leaks when passing an array buffer from c++ to js.
The callback given to the ArrayBuffer only gets called on program close, and not when js no-longer references the data.
For a long running program, this doesn't really work.

test.js:

const {GetData} = require("./build/Debug/addon.node");

while (true) {
    // Get more data
    var mydata = GetData();
    console.log(mydata);
}

console.log("JS test finished");

Relevant parts of c++

unsigned char* GetData(size_t* packetSize) {
        *packetSize = 6;
        unsigned char* tempPacket = (unsigned char*)malloc(sizeof(*packetSize));

        tempPacket[0] = 0;
        tempPacket[1] = 1;
        tempPacket[2] = 2;
        tempPacket[3] = 3;
        tempPacket[4] = 4;
        tempPacket[5] = 5;

        return tempPacket;
}

void cleanupHook (Napi::Env env, void* arg) {
    std::cout << "Freed some data" << std::endl;
    free(arg);
}

Napi::ArrayBuffer GetDataWrapper(const Napi::CallbackInfo& info) {
    Napi::Env env = info.Env();
    size_t packetSize;

    unsigned char* data = GetData(&packetSize);

    // Data is freed by the callback cleanupHook
    Napi::ArrayBuffer array =  Napi::ArrayBuffer::New(env
                                                      ,(void*)data
                                                      ,packetSize
                                                      ,cleanupHook
                                                      );

    return array;
}

[mhdawson]

Are you sure the GC is running and freeing the objects?

There is no guarantee that the gc will run unless is needs memory. It's a classic problem that if you use lots of C++ memory tied to a objects without using a lot of memory on the heap (ie JS objects not counting the C++ memory) then the gc never needs to run and the C++ memory does not get freed.

You can try to use:

inline int64_t MemoryManagement::AdjustExternalMemory(Env env, int64_t change_in_bytes) {
  int64_t result;
  napi_status status = napi_adjust_external_memory(env, change_in_bytes, &result);
  NAPI_THROW_IF_FAILED(env, status, 0);
  return result;
}

to let the gc know that objects are holding a lot of external memory alive. You need to adjust upwards when you crate the buffer and then adjust downwards in the cleanupHook.

[AlexVdberg]

Adjusting the external memory doesn't seem to help.
cleanupHook() is not getting called, which tells me gc still isn't running with the adjusted memory.

I added the following to GetDataWrapper():

    uint64_t res = Napi::MemoryManagement::AdjustExternalMemory(env, 6);
    std::cout << "Adj up res: " << res << std::endl;

And the following to cleanupHook():

    uint64_t res = Napi::MemoryManagement::AdjustExternalMemory(env, -6);
    std::cout << "Adj down res: " << res << std::endl;

[mhdawson]

Is the memory you are allocating just 6 bytes long?

Do you have files for a full recreate that we can just run to look at the behaviour?

[gabrielschulhof]

@mhdawson I think the reason why it's not finalizing is that the Node-API finalizer attempts to call the user's finalizer via SetImmediate() – which will never happen when running in a tight loop.

[gabrielschulhof]

https://github.com/nodejs/node/blob/d63b7170443b74de68703e98f4594529d057a4cb/src/node_api.cc#L65-L86

[gabrielschulhof]

The sequence is this: napi_create_external_arraybuffer() calls napi_create_external_buffer() which creates a node::Buffer and attaches v8impl::BufferFinalizer::FinalizeBufferCallback to the weak reference.

[gabrielschulhof]

@addaleax ^

[gabrielschulhof]

nodejs/node@3ca0df2 introduces the SetImmediate().

[gabrielschulhof]

@mhdawson looks like the reason @addaleax added the SetImmediate() around the user's finalizer is to retain the ability to call into JS – an ability that is absent during the initial gc. Interestingly we fixed this with SecondPassCallback for regular references. I wonder if we could do the same here, and avoid the SetImmediate. Otherwise, this becomes another issue that could only be fixed by nodejs/node#30327.

[mhdawson]

@AlexVdberg

Can you confirm that changing

while (true) {
    // Get more data
    var mydata = GetData();
    console.log(mydata);
}

to

function getNextData {
  var mydata = GetData();
  console.log(mydata);
  setImmediate(getNextData)
}

Resolves the problem. If so that would confirm what @gabrielschulhof mentions as potentially contributing to what you are seeing.

[addaleax]

@mhdawson I think the reason why it's not finalizing is that the Node-API finalizer attempts to call the user's finalizer via SetImmediate() – which will never happen when running in a tight loop.

I would consider this expected behavior then. Switching to SecondPassCallback() does not fix anything per se because of the lack of guarantees around GC timing – which is to say, users should never rely on exact timing anyway.

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.