src,n-api: throwing error with napi_pending_exception status

[legendecas]

It should be napi_pending_exception if an JavaScript error is
pending to be handled. Or it might have a great chance to be ignored.
See https://github.com/nodejs/node-addon-api/blob/master/napi-inl.h#L2006-L2013

Related: #29768 (comment)

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[legendecas]

Maybe JavaScript error should not be thrown here and returning napi_function_expected instead?

[gabrielschulhof]

That would be a semver-major change, I think, so we mustn't.

[gabrielschulhof]

Also, we cannot change the return status from napi_function_expected to napi_pending_exception because that's semver-major as well.

[mhdawson]

@legendecas its hard to tell from the diff. Are the functions being changed still experimental?

[legendecas]

@mhdawson its hard to tell from the diff. Are the functions being changed still experimental?

No, they are not. The PR was trying to address the issue that thrown exceptions were possibly ignored in actual usage like in node-addon-api. Since semantically no exception were expected on non-napi_pending_exception status.

Or we could update node-addon-api to not ignore the JS exception whatever the status is. But it might be ambiguous since there might be two errors to be handled: one for the error status, one for the thrown JS exception.

[gabrielschulhof]

@legendecas Unfortunately we cannot change what status a function returns under what conditions because that would be semver-major. In this case, because we're talking about N-API, semver-major does not mean that's it's OK to release with the next major version of Node.js, but that it's OK to release with the next major version of N-API.

Let's label this PR, even if closed, to remind us of the changes we need to make if/when we decide to release a version of N-API with semver-major changes.

[gabrielschulhof]

That would be a semver-major change, I think, so we mustn't.

[gabrielschulhof]

Also, we cannot change the return status from napi_function_expected to napi_pending_exception because that's semver-major as well.

[gabrielschulhof]

@legendecas I think in node-addon-api we can handle those N-API calls we know throw exceptions as special cases, and smooth over the differences in behaviour there to avoid ignoring exceptions and/or throwing double exceptions.

Unfortunately this non-uniformity has crept into N-API. We'll have to be vigilant in the future to avoid it from affecting newly added N-APIs.

[gabrielschulhof]

@legendecas Additionally, N-APIs strong ABI stability guarantees mean that we must not change the return values of a function in response to certain error conditions.

[legendecas]

The only place that used this macro is napi_create_typedarray by CREATE_TYPED_ARRAY.

[mhdawson]

My question about experimental status was directly related to what @gabrielschulhof has commented. We can't change the behavior of non-experimental APIs.

[legendecas]

Yes, I'd agree that changing the behavior of non-experiment N-APIs would hardly be acceptable.

Still, it would be nice to mark these macros that not aligned with the expected design goals as deprecated to prevent possible future maintenance/new-APIs from using these macros.

[legendecas]

Closing for non-experimental behavior changes and I'd like to propose another one with updating the comments or so to mark the macros as deprecated from been used in new N-APIs.