Memory leak on script crash

[Gautham B A]

Hi all,

I'm observing a memory leak when the script crashes at runtime. Here the JavaScript code (made to crash on purpose by throwing Exception).

var res = new N1qlQuery('SELECT * FROM `sample`');
throw 'error'; // purposefully crash.

The corresponding C++ code for N1qlQuery is as follows -

void N1qlQuery(const v8::FunctionCallbackInfo<v8::Value> &args) {

  v8::Isolate *isolate = v8::Isolate::GetCurrent();

  v8::HandleScope handleScope(isolate);

  v8::Local<v8::Name> query_name = v8::String::NewFromUtf8(isolate, "query");

  v8::Local<v8::Value> empty_string = v8::String::NewFromUtf8(isolate, "");  

  v8::Local<v8::ObjectTemplate> obj = v8::ObjectTemplate::New();
  obj->Set(query_name, args[0]);

  

  args.GetReturnValue().Set(obj->NewInstance());

}

I believe obj->NewInstance() is causing the memory leak, because I've observed no memory leak when the last line in C++ was commented out, even if the JavaScript crashed.

There is no memory leak when the script finishes execution without crashing.

Could anyone please tell me the right way to expose a JavaScript class? (perhaps by avoiding a call to NewInstance() in C++).

Thanks,

--Gautham

[Ben Noordhuis]

You should post full code if you want an answer but if I had to guess,
it's that you have a `if (try_catch.HasCaught()) exit();` or the
equivalent thereof in your code somewhere - i.e., termination without
proper shutdown of the VM.

Aside: you can use `args.GetIsolate()` instead of
`Isolate::GetCurrent()`, it's a little faster, and you don't need a
HandleScope, API callbacks have one implicitly.

[Gautham B A]

Thank you Ben.

Yes, there's a try_catch.HasCaught() before running the script in order to prevent the process from exiting.

Here's the code that will cause the script to run -

int SendUpdate(std::string value, std::string meta,

                         std::string doc_type) {

  v8::Locker locker(GetIsolate());

  v8::Isolate::Scope isolate_scope(GetIsolate());

  v8::HandleScope handle_scope(GetIsolate());

  v8::Local<v8::Context> context =

      v8::Local<v8::Context>::New(GetIsolate(), context_);

  v8::Context::Scope context_scope(context);

  v8::TryCatch try_catch(GetIsolate());

  v8::Handle<v8::Value> args[2];

  if (doc_type.compare("json") == 0) {

    args[0] =

        v8::JSON::Parse(v8::String::NewFromUtf8(GetIsolate(), value.c_str()));

  } else {

    args[0] = v8::String::NewFromUtf8(GetIsolate(), value.c_str());

  }

  args[1] =

      v8::JSON::Parse(v8::String::NewFromUtf8(GetIsolate(), meta.c_str()));

  if (try_catch.HasCaught()) {

    last_exception = ExceptionString(GetIsolate(), &try_catch);

    LOG(logError) << "Last exception: " << last_exception << '\n';

  }

  v8::Local<v8::Function> on_doc_update =

      v8::Local<v8::Function>::New(GetIsolate(), on_update_);

  on_doc_update->Call(context->Global(), 2, args);

  if (try_catch.HasCaught()) {

    LOG(logDebug) << "Exception message: "

                  << ExceptionString(GetIsolate(), &try_catch) << '\n';

    

    return ON_UPDATE_CALL_FAIL;

  }

  return SUCCESS;

}

Is it possible to reclaim the memory without shutting the VM down?

[Ben Noordhuis]

Depends on what you mean by 'reclaim' and 'memory leak' - memory isn't
reclaimed until the garbage collector deems it necessary. People
often mistake that for a memory leak when it is in fact the garbage
collector doing its work (or rather, doing as little work as it can
get away with.)

I don't see an actual memory leak in the code you posted but creating
a new ObjectTemplate for every query isn't very efficient. If you
don't use ObjectTemplate features, replace it with Object::New().

[Gautham B A]

Yes, the memory leak in this code isn't visible as it appears. 

I observed the memory leak when I ran the "top" command. But when I commented out the call to "NewInstance()", there wasn't any growth in memory (which is obvious because there's nothing being created and passed onto the JS world).

[Brendan Bates]

Hey Gautham B A,

I am currently experiencing a similar issue.  I have found that using NewInstance() causes a slow memory growth/leak in our application.  We aggressively call LowMemoryNotification() 

on our scripts, as they are meant to run for a very long time and accept the performance tradeoff.  This should theoretically clean up stray objects, but it seems that objects created via

NewInstance() are never properly marked for GC (maybe, I don't really know what's being held).  What's odd is that I can create a weak persistent beneath one of these objects, and they

get cleaned up regularly in the memory notification calls.

Are you still having issues?  I might make a test program and throw it on a gist and report back here.  I'd be curious as this doesn't seem like acceptable behavior for the engine.

I don't see an actual memory leak in the code you posted but creating 
a new ObjectTemplate for every query isn't very efficient. 

Just wanted to reply to this to say, that is an irrelevant point to this discussion.  Even if you are using it sparingly, if you have a long-running script that calls NewInstance(), then

this would be a problem.

[Gautham B A]

Hi Brendan,

I managed to design a work around for this issue. I moved the class that I was trying to expose from C++ world to JavaScript - https://github.com/couchbase/eventing/blob/master/v8_consumer/src/builtin.js

By doing this, whenever a call to "new N1qlQuery()" is made, we force the instantiation to be done in the JavaScript world, as opposed to calling NewInstance() which would create it in the C++ world. Thus, we get rid of the call to NewInstance() which causes the memory leak problem.

The methods for the class above - iter(), execQuery() and stopIter() are exposed from C++ to complete the class definition.

To summarize, just get rid of the call to NewInstance(). As a substitute, write the class to be exposed in JavaScript and expose the corresponding member functions from C++.

Thanks,

--Gautham

[Gautham B A]

@Brendan, could you please post how you managed to overcome this issue?

[Ben Noordhuis]

On Mon, May 1, 2017 at 4:25 PM, Brendan Bates <brendan...@gmail.com> wrote:
>> I don't see an actual memory leak in the code you posted but creating
>> a new ObjectTemplate for every query isn't very efficient.
>
> Just wanted to reply to this to say, that is an irrelevant point to this
> discussion. Even if you are using it sparingly, if you have a long-running
> script that calls NewInstance(), then
> this would be a problem.

Seems relevant enough to me if you like efficient programs but I take
back my words because there is a memory leak: ObjectTemplate::New()
produces a template that caches instantiations. The first
instantiation is cached, subsequent instantiations are cloned from
that cached instance.

It's not an issue when you create the ObjectTemplate once but it does
become an issue when you create the ObjectTemplate afresh every time.
That is a silly thing to do, though; it misses the point of templates.

[Brendan Bates]

Seems relevant enough to me if you like efficient programs

Oh I definitely agree it's important, just that the topic on hand isn't necessarily the performance impact but the memory usage.  In our case, we accept the performance

impact for the flexibility of the solution.

 It's not an issue when you create the ObjectTemplate once but it does become an issue when you create the ObjectTemplate afresh every time. 

I have a question in response to this then.  How would you handle a dynamic multi-indexed field mapped to a C++ object?  Currently we have a root ObjectTemplate with an

indexed property handler that knows it is the first of three indices.  The handler when requested will then generate a new ObjectTemplate with an indexed handler that knows it is the

second of three indices, etc.  So yes, every call to "var test = data[1][2][3]" will generate three object templates as it goes up the chain.  Perhaps this is not possible/best use of v8, as a function call for fetching the object could accomplish this.  However for our system, that is less declarative than just using array notation.  Unfortunately a stock object does not provide a way of setting an indexed property handler (as far as I know).

As an aside to all of this - is this considered a bug?  I feel like if not, the repercussions of this should be documented somewhere such as the NewInstance call in the v8 API.  I would not

have caught this if we didn't run some stress tests on our system.  I wrote a test that generates tens of MB per second (even when calling LowMemoryNotification) from using this behavior.

[Brendan Bates]

Also @Gautham B A we have not figured out a good solution yet - still reviewing the best way to actually get around this.

[Ben Noordhuis]

On Mon, May 1, 2017 at 5:20 PM, Brendan Bates <brendan...@gmail.com> wrote:
> I have a question in response to this then. How would you handle a dynamic
> multi-indexed field mapped to a C++ object? Currently we have a root
> ObjectTemplate with an
> indexed property handler that knows it is the first of three indices. The
> handler when requested will then generate a new ObjectTemplate with an
> indexed handler that knows it is the
> second of three indices, etc. So yes, every call to "var test =
> data[1][2][3]" will generate three object templates as it goes up the chain.

I'd create a single ObjectTemplate with one or more internal fields
for stashing data, cache the template in a Persistent<ObjectTemplate>
and instantiate it three times. The indexed property handlers then
look at the internal fields to figure out what they should
access/return.

> As an aside to all of this - is this considered a bug? I feel like if not,
> the repercussions of this should be documented somewhere such as the
> NewInstance call in the v8 API.

I'm not a V8 maintainer so I can't answer this but to me it seems like
reasonable behavior. The idea of a template is to share behavior
across multiple instances.

A CL updating include/v8.h to explain the memory ramifications will
almost certainly be accepted.

[Brendan Bates]

I'm not a V8 maintainer so I can't answer this but to me it seems like 
reasonable behavior.  The idea of a template is to share behavior 
across multiple instances. 

I guess the only reason I would consider it a bug: shouldn't any Javascript local/persistent be fully cleaned up when it goes out of

scope?  If you create the Local<ObjectTemplate> in a HandleScope and the scope closes, you can't use that template anymore.  I

would also think any metadata or cache related to that scope would clear as well.  I honestly don't know enough about v8 to make

that assumption, but I assumed most things stored in a Local were memory leak safe.

[Brendan Bates]

Just implemented the changes you suggested.  Works great and I will continue doing this in the future!  The memory

is WAY more stable and I now further understand the point of the ObjectTemplate now.  I did still report this in the issue

tracker, as I hope at least the effect of NewInstance() is documented (or made more visible if it already exists).

To Gautham B A, this problem can be solved by using an ObjectTemplate and persisting and reusing it instead of creating

a new one every time needed.