src: add HandleScope to fix error

[ryzokuken]

Add HandleErrors to the AsyncScope constructor and destructor in
async_wrap-inl.h to fix "FATAL ERROR" incurring observed while running
test-http2-respond-with-file using the --trace-events-enabled flag.

Fixes: #19921

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

cc @addaleax @jasnell

[ryzokuken]

I'm also testing this locally. Meanwhile, could you help me understand what exactly was wrong and how it was fixed by adding the two HandleScopes? That'd help me get started towards contributing more towards this part of the project.

[ryzokuken]

The error says: FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope, so it makes sense that making a HandleScope would solve a thing or two, but where was HandleScope::CreateHandle() called in the first place?

[addaleax]

Meanwhile, could you help me understand what exactly was wrong and how it was fixed by adding the two HandleScopes?

@ryzokuken When accessing JS values from C++, what the engine provides in order to to that is a handle object, i.e. Local<Value> or some more specific kind of Local<>, that refers to the JS value.

As an optimization, these handles do not directly manage the lifetime of JS objects, like one might expect from C++ objects. Instead, V8 uses the concept of a handle scope: These form a stack, and new handles are always added to the scope on top of the stack. That scope is basically a GC root – all JS objects that are registered with it are kept alive, until the scope is closed.

In Node.js, if our code is called from e.g. libuv, we need such a scope that is inside of the callback. Otherwise, any JS object we’re creating or getting a reference to in some other way, would be registered in a scope that is outside of the event loop. That would mean that these objects cannot be garbage-collected until the event loop finishes, creating a memory leak.

(Having an error instead of a memory leak was implemented because it made checking for these kinds of leaks inside Node.js easier, btw.)

but where was HandleScope::CreateHandle() called in the first place?

According to the stack trace from #19921, that would be the Number::New call inside Emit() in async-wrap.cc.

It might make more sense to add the handle scope there, because that’s where they are being used. However, while HandleScopes are already pretty light-weight, that’s a hot path and we’d need to benchmark such a change.

[BridgeAR]

CI https://ci.nodejs.org/job/node-test-pull-request/14268/

[jasnell]

Landed in 6eaeabc

[hashseed]

Guess I saw this too late. Current solution works. But I would just put a v8::HandleScope in Emit.

[ryzokuken]

@hashseed yes, that sounds great too, but I wonder: do we need v8::HandleScope for all calls to Emit? If not, then perhaps it makes more sense to manually call if like we've done?

[hashseed]

Yes. Emit allocates via Number::New. If the caller has not created a handle scope, this will crash. Creating a handle scope is fairly cheap, so it simply makes sense that the function that performs the allocation also provides the handle scope, and does not defer to the caller to fulfill this implicit contract.

[ryzokuken]

@hashseed that makes a lot of sense. Forgive me for the noobish question, but what if v8::HandleScope is called twice?

[addaleax]

@ryzokuken You end up with two nested HandleScopes, and all handles inside the inner one will be registered only with that one and will be made invalid once that scope is left, even though there is still the outer one.

If you create two handle scopes immediately after one another, the effect will be the same as if there was only one – there just won’t be any handles associated with the outer one.

[hashseed]

What Anna said. Having a nested handle scope is fine.

[ryzokuken]

@addaleax @hashseed In that case, calling v8::HandleScope right inside Emit won't break anything. We could do that and then remove any redundant calls to v8::HandleScope, but nothing will break even if we somehow miss some. On the other hand, this would ensure that there won't be any accidental crashes in the future just because someone missed a v8::HandleScope call before calling Emit (exactly what happened here, right?)

So, what do you say? Should I also add a call inside Emit, then set out to remove redundant calls?

[addaleax]

I mean, if @hashseed says it’s fine from a performance point of view, I believe that. :) You can feel free to make that switch.

[hashseed]

Sounds like a plan :)

[ryzokuken]

@addaleax @hashseed Great! Will make a PR first thing in the morning tomorrow (it's late here now).