N-API: memory leak with call to napi_create_function

Version: v12.8.0
Platform: Linux atc 5.1.20-300.fc30.x86_64 #1 SMP Fri Jul 26 15:03:11 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux
Subsystem:

This is again with respect to the node-oracledb driver (Oracle Database driver for Node.js). I have adjusted the code once again to eliminate any requirement for an Oracle database. I'll attach the test case shortly after this issue is created.

For Oracle Objects I have created a class dynamically at run-time based on the name. This can be seen in the test case as a call to conn.getDbObjectClass(). Internally, the call to conn.getDbObjectClass() calls into a C module which then makes a call to conn._getDbObjectClassJS(). This function then builds the class and stores the result in a cache on the connection called _dbObjectClasses.

When the connection is closed by a call to conn.close(), I have to iterate over the entries in the cache and deliberately break the prototype chain; otherwise, the classes that are built are never garbage collected.

But, even if I do that and the classes are garbage collected, there is still a memory leak. The pattern shows that there are periods of relatively stable memory usage followed by jumps in memory usage. The periods of stable memory usage grow longer as the number of iterations increases but the jump in memory also increases. It would seem that a list of some kind is being populated with intermittent, increasing size allocation. Looking at the heap memory in the Chrome development tools indicates that the memory is all found in noscript_shared_function_infos -- but I'm not sure what that means! Interestingly, though, if I remove the call to napi_define_properties in njsDbObjectType_populate(), the memory leak goes away.

I'm not sure if this is related to the fix introduced in PR #27805 or not, but version 12.0 is the first version that has this issue. Prior to 12.0 the code suffers from the memory leak that that issue corrected.

I hope this is sufficient to discover the source of this issue. Let me know if you need anything else!

The test case can be seen in the zip file. Included is a sample stats.txt that demonstrates the memory leak.

Run the tests with

NODE_PATH=$PWD/lib node --expose-gc demo.js

I just tried this with Node.js 12.8 (just released) and the issue is still there, unfortunately.

Interestingly, a call to napi_define_properties() is not required. A simple call to napi_create_function(), even if that object is never tied to any other object, is all that is required.

Here is a much simplified package that contains only a single method called leak_memory() and all it does is calls napi_create_function(). It leaks memory quite rapidly!

issue_28988_simplified.zip

Run it with

node --expose-gc demo.js

You can wait until it finishes or stop it after a few hundred thousand iterations and then examine the file stats.txt, or you can look at the output from the top command.

Here is a much simplified package that contains only a single method called leak_memory() and all it does is calls napi_create_function(). It leaks memory quite rapidly!

issue_28988_simplified.zip

Run it with
node --expose-gc demo.js
You can wait until it finishes or stop it after a few hundred thousand iterations and then examine the file stats.txt, or you can look at the output from the top command.

Major objects that retained in heap in this case were TickObject which would gets collected once they was been ran.

With changing the while loop in function run to looping with setInterval, the seemingly major leaking behavior had just gone.

Though there still exists a minor heap size increasing on noscript_shared_function_infos.

Which version did you use? I am using Node.js 12.8. I modified demo.js to look like this:

const issue_28988 = require('./build/Release/issue_28988.node');
const fs = require('fs');

const statsFileName = "stats.txt";
const maxIters = 2915000;

let numIters = 0;

function performIter() {
  for (let i = 0; i < 1000; i++) {
    numIters++;
    issue_28988.leak_memory();
  }
  console.log("Processed", numIters, "iterations...");
  global.gc();
  const stats = process.memoryUsage();
  const text1 = `${numIters},${stats.rss},${stats.heapTotal},`;
  const text2 = `${stats.heapUsed},${stats.external}\n`;
  fs.appendFileSync(statsFileName, text1 + text2);
}


async function run() {
  fs.writeFileSync(statsFileName, "Num Iters,RSS,Heap Total,Heap Used,Ext\n");
  setInterval(performIter, 0);
}

run()

and I didn't see a whole lot of difference between the two. I see a leak of about 12 bytes/iteration but there are periods of stability followed by a jump. The periods of stability lengthen but the jumps also get bigger. 12 bytes doesn't seem like a lot....until you realize that if you build a class with a number of getter/setter and specialized functions that adds up quickly. If you adjust the call to the leak_memory function to do the following instead:

    for (let j = 0; j < 20; j++) {
      issue_28988.leak_memory();
    }

then the leak jumps to about 250 bytes/iteration. This quite closely matches the behaviour I was seeing with the full test case which probably calls napi_create_function() internally about 20 times.

@gabrielschulhof

I seem to remember @gabrielschulhof mentioning that V8 never collects functions but I'll have to confirm if that is the case and if we have it documented somewhere.

EDIT: functions created in native code that is.

@gabrielschulhof

I think @gabrielschulhof is still away for another week and I've not been able to find documentation or earlier issues where that was discussed @hashseed can you confirm one way or the other if V8 collects functions created in native code?

V8 has something called “template instantiations cache” whose entries refer back to the Function instances indexed by their FunctionTemplates – I could imagine the underlying issue here is that those entries are not cleared when the FunctionTemplate is garbage collected. But this may require a V8 person to confirm and fix.

@verwaest

@verwaest knows the details here.

Yeah I think we keep instantiated functions around for faster instantiations.

@verwaest

@verwaest can you confirm

Function template::New results in cached templates, whereas Function::New creates an uncached template.

Cached here doesn't mean it's a weak pointer: in the browser for correctness they /need/ to be unique instances that have identity. Since they are mutable objects like any other it's observable, and the fact that they aren't instantiated eagerly is simply a memory and performance optimization. So the cache isn't there (just) for performance, it's actually needed for correctness in the browser.

This cached way of using templates is possibly not what you want. Doesn't Function::New fit your usecase?

This cached way of using templates is possibly not what you want. Doesn't Function::New fit your usecase?

I am not using the C++ API directly, but indirectly through N-API. Is there a way to create a function with Function::New() using N-API?

@verwaest

@verwaest, @hashseed can you help confirm if what I've seen matches expected behaviour or not?

@mhdawson

@mhdawson I ran the following code as both a Node.js addon and as a V8 cctest:

// ---8<------------------------------------------------------------------------
struct WeakRefCounters {
  inline void inc_created() {
    created++;
    maybe_print();
  }
  inline void inc_collected() {
    collected++;
    maybe_print();
  }
  inline void maybe_print() {
    if (!(created % 1000)) {
      fprintf(stderr, "%lu - %lu = %lu\n", created, collected,
        created - collected);
    }
  }
  size_t created;
  size_t collected;
};

class Ref {
 public:
  Ref(v8::Isolate* isolate,
      v8::Local<v8::Function> fn,
      WeakRefCounters* counters):
    pers(isolate, fn), counters(counters) {
    pers.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
  }
  ~Ref() {
    pers.Reset();
    counters->inc_collected();
  }
  static void WeakCallback(const v8::WeakCallbackInfo<Ref>& info) {
    delete info.GetParameter();
  }
  v8::Persistent<v8::Function> pers;
  WeakRefCounters* counters;
};

static void DummyFunction(const v8::FunctionCallbackInfo<v8::Value>& info) {}

// Entry point. Create a new function and track its life cycle.
static v8::Local<v8::Function>
NewFunction(v8::Isolate* isolate, WeakRefCounters* counters) {
  v8::Local<v8::Function> fn =
    v8::Function::New(isolate->GetCurrentContext(), DummyFunction)
      .ToLocalChecked();

  counters->inc_created();
  new Ref(isolate, fn, counters);

  return fn;
}
// ---8<------------------------------------------------------------------------

I wrote the following V8 test:

#include "src/execution/isolate.h"
#include "src/heap/factory.h"
#include "src/objects/name-inl.h"
#include "src/utils/ostreams.h"
#include "src/objects/objects.h"
#include "test/cctest/cctest.h"

namespace v8 {
namespace internal {

TEST(FunctionLeak) {
  WeakRefCounters counters = { 0, 0 };
  CcTest::InitializeVM();
  Isolate* isolate = CcTest::i_isolate();
  HandleScope scope(isolate);
  v8::Isolate* v8_isolate = reinterpret_cast<v8::Isolate*>(isolate);

  for (size_t i = 0;; i++) {
    HandleScope inner_scope(isolate);
    NewFunction(v8_isolate, &counters);
    if (!(i % 10000)) {
      CcTest::CollectGarbage(i::NEW_SPACE);
      CcTest::CollectAllGarbage();
    }
  }
}

}  // namespace internal
}  // namespace v8

I also wrote a corresponding Node.js native addon test:

#include <stdio.h>
#include <node.h>

static void FunctionFactory(const v8::FunctionCallbackInfo<v8::Value>& info) {
  info
    .GetReturnValue()
    .Set(NewFunction(
      info.GetIsolate(),
      static_cast<WeakRefCounters*>(info.Data().As<v8::External>()->Value())));
}

static void DeleteCount(void* data) {
 delete static_cast<WeakRefCounters*>(data);
}

NODE_MODULE_INIT() {
  v8::Isolate* isolate = context->GetIsolate();  
  WeakRefCounters* refcounts = new WeakRefCounters({0, 0});
  node::AddEnvironmentCleanupHook(isolate, DeleteCount, refcounts);
  exports->Set(context,
               v8::String::NewFromUtf8(isolate,
                                  "functionFactory",
                                  v8::NewStringType::kNormal).ToLocalChecked(),
               v8::Function::New(context,
                                 FunctionFactory,
                                 v8::External::New(isolate, refcounts))
                   .ToLocalChecked())
      .FromJust();
}

const leak = require('bindings')('leak');

function makeNewFunction() {
  leak.functionFactory();
}

while(true) {
  makeNewFunction();
}

I ran the V8 test with

./cctest --max-heap-size=8 test-function-leak/FunctionLeak

and the Node.js addon with

node --max-heap-size=8 ./index.js

The V8 test did not seem to leak, whereas the Node.js addon ran out of heap space very quickly.

Now, it's true that I ran the V8 test on its master, rather than on whatever version of V8 we have in Node.js master, so the leak may have been fixed. Alternatively, it may be that the settings with which we compile and launch V8 may be causing it to leak. I'll try and figure out how to make sure that the version of V8 I'm building separately is exactly the same as the version of V8 in the Node.js master source tree.

@mhdawson

@mhdawson looks like this is a problem that was fixed. Looking at deps/v8/ChangeLog, I found that the version in master is 2.7.299. When I checked out that version of V8 in its tree and re-ran the cctest, it also ran out of heap space. So, looks like this'll be fixed when we upgrade past the fix in V8.

Check that bug https://bugs.chromium.org/p/v8/issues/detail?id=9674

@gabrielf

@gabrielf which version of V8 will the change be in? We are already at 7.7 in 12.x but I'm not sure I've 7.8 will be backported (@targos can you comment) and due to having upped the mininum OSX devtools level to accommodate later V8 versions in 13.x my guess is that 7.9 and later most likely will not be backported.

We may need to float the fix.

@gabrielschulhof

Talked to @gabrielschulhof, sounds like the fix is in 7.8 and @targos and @addaleax are going to try to land that in 12.x #29694 (comment). If that does not pan out we may need to look at floating the relevant fix.

@targos

@targos is there any chance that 7.8 will go into 12.x as a SemVer minor or do we need to look to see if we can float the change that @gabrielschulhof mentioned fixes this?

is there any chance that 7.8 will go into 12.x as a SemVer minor

Maybe, if someone can help to do the ABI compat patch

I opened #30109 to make it more visible

Current state:

Node.js branch	V8 version	good/bad
v10.x-staging	6.8.275	✗
v12.x-staging	7.8.279	✓
v14.x-staging	8.1.307	✓

Since v10.x is going into maintenance it's unlikely that a new major version of V8 will be backported to it. So, this memory leak will go away completely only when we drop support for v10.x.

@gabrielschulhof

@gabrielschulhof I wonder if we should add a test that validates this is not regressed at some point or do we think it should be covered by the testing on the V8 side?

@mhdawson

@mhdawson IMO this is internal to V8.

@gabrielschulhof

@gabrielschulhof thanks.

It looks like this is complete to me since 10.x is out of service. I'm going to close the issue. Is you feel that was not the right thing to do please re-open.

Navigation Menu

Repository navigation

N-API: memory leak with call to napi_create_function #28988

11 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

N-API: memory leak with call to napi_create_function #28988

Description

Activity

anthony-tuininga commented on Aug 6, 2019

anthony-tuininga commented on Aug 6, 2019

anthony-tuininga commented on Aug 9, 2019

anthony-tuininga commented on Aug 9, 2019

legendecas commented on Aug 11, 2019

anthony-tuininga commented on Aug 12, 2019

mhdawson commented on Aug 14, 2019

mhdawson commented on Aug 14, 2019

addaleax commented on Aug 14, 2019

hashseed commented on Aug 15, 2019

mhdawson commented on Aug 26, 2019

verwaest commented on Aug 26, 2019

anthony-tuininga commented on Aug 26, 2019

11 remaining items

mhdawson commented on Sep 4, 2019

gabrielschulhof commented on Sep 24, 2019

gabrielschulhof commented on Sep 24, 2019

Alxspb commented on Sep 24, 2019

mhdawson commented on Sep 30, 2019

mhdawson commented on Oct 10, 2019

mhdawson commented on Oct 24, 2019

targos commented on Oct 25, 2019

targos commented on Oct 25, 2019

gabrielschulhof commented on Apr 17, 2020

mhdawson commented on Apr 17, 2020

gabrielschulhof commented on May 1, 2020

mhdawson commented on May 4, 2020

mhdawson commented on Feb 17, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions