Explicitly restrict escapable scope to escape just one value

[RReverser]

Allowing an arbitrary number of escaped handles makes scope management more complicated in implementations as scopes are otherwise easily stored directly on a stack or in a vector-based stack implementation. In either case, it's problematic to reserve space for an unknown number of values being inserted into the outer scope.

Even V8 allows to call EscapableHandleScope::Escape only once according to docs, but this is not mentioned in N-API.

There are at least two ways to handle this:

• Document the restriction. Still doesn't guarantee that someone won't try escaping more than one value, which would be an undefined behaviour, but better than nothing. (done)
• Explicitly limit escaped values to just one by merging napi_escape_handle into napi_close_escapable_handle_scope as an extra parameter. Given that the most usual case for escaping handles is exactly to return a value outside of a function, it's usually anyway happening at the end, and if not, it's not a problem to store the handle in a variable and pass it to napi_close_escapable_handle_scope when it's called. This will allow no mistakes about the allowed number of escapes.

[jasongin]

This was just implemented at nodejs/node#13651

[RReverser]

@jasongin That PR adds a check to Node.js-on-V8 implementation, while this issue is a continuation of discussion about better visibility of such restriction.

[RReverser]

Ah cool, that's great. Still, I'm somewhat keen on exploring the napi_close_escapable_handle_scope suggestion if it turns out to be feasible.

[RReverser]

Updated the issue description.

[jasongin]

That PR added a test; the same tests are used to verify the behavior of N-API implementations for any JS engine.

The documentation was actually updated in a different PR... I'll find it.

[jasongin]

Doc update: nodejs/node#13650

[RReverser]

Yeah, I believe that documentation update by @mhdawson was related to the meeting discussion, I'm just slow to raise issues I promised to :)

[mhdawson]

Yes I just wanted to start by documenting the current limitation. Having the discussion on whether going further and tying it into the close is the next step.

One thought I had is that with the current approach its easy to relax the limitation (if for example all JS engines can escape more than one object at some point in the future) through a doc update, were as limiting the escape to the close would require additional API to relax at a later time. The V8 apis also include the method to escape not tied to the close so maybe there was a use case where that was needed.

[RReverser]

Given that API is more or less final now, is the conclusion here that this won't be done on API level in addition to docs change? If so, I guess I can close this issue.

[mhdawson]

@RReverser at this point I think it will stay as is so you can close the issue.