Introduce status napi_cannot_call_into_js

[gabrielschulhof]

napi_call_function() and other N-APIs that use NAPI_PREAMBLE() return napi_pending_exception when env->can_call_into_js() is no longer true. We should distinguish between a pending exception and an inability to call into JS.

Re: nodejs/node-addon-api#591

[addaleax]

We should distinguish between a pending exception and an inability to call into JS.

How would programs treat these two differently?

[gabrielschulhof]

@addaleax if we return napi_pending_exception then napi_get_and_clear_last_exception() should not return a JS undefined as a napi_value, because the last exception should not be empty. Currently the crash at the linked issue is happening because napi_get_and_clear_last_exception() is called in response to the napi_pending_exception return status, and, ultimately, an attempt is made to create a reference to the JS undefined value, which is not possible with N-API, because references can only be made to JS objects.

[gabrielschulhof]

This second failure is treated as a fatal error.

[gabrielschulhof]

@addaleax Here's a possible distinction: An exception can be cleared, and calls can once more be made into JS, whereas if an inability to call into JS is reported, then there is no way to clear that up. Good, because, currently that means the environment is shutting down.

[gabrielschulhof]

@addaleax I'm starting to think that perhaps this should not be semver-major, nor n-api-semver-major, but rather a bug that requires a semver-minor fix (the introduction of the new status). If you agree I will remove the labels and start a PR.

[mhdawson]

@gabrielschulhof is there a reasonable recovery path if you make a call and env->can_call_into_js() is false? In other words is it reasonable for code to make calls in this situation and ignore the failure or is it a bug in the code? If it's a bug I'm wondering if we should call napi_fatal_error() ?

[addaleax]

@gabrielschulhof is there a reasonable recovery path if you make a call and env->can_call_into_js() is false? In other words is it reasonable for code to make calls in this situation and ignore the failure or is it a bug in the code?

It’s not a bug in the code, it’s something that an application should handle gracefully (by not making any more N-API calls).

[gabrielschulhof]

@gabrielschulhof is there a reasonable recovery path if you make a call and env->can_call_into_js() is false? In other words is it reasonable for code to make calls in this situation and ignore the failure or is it a bug in the code?

It’s not a bug in the code, it’s something that an application should handle gracefully (by not making any more N-API calls).

Right, this is why I believe we should properly inform the application by distinguishing cannot-call-into-JS-anymore with its own status, rather than returning napi_pending_exception, and this is why I believe

  RETURN_STATUS_IF_FALSE((env),                                     \
      (env)->last_exception.IsEmpty() && (env)->can_call_into_js(), \
      napi_pending_exception);                                      \

is a bug.

@mhdawson presumably the addon could silently release whatever resources is holding. I don't believe we should call napi_fatal_error().

[mhdawson]

@gabrielschulhof, @addaleax got it. I agree a separate status is needed so that the application knows what to do along with additional documentation in the error handling section which explains this case and what you should do when you see it.

From what I understand so far it does share some similarities with pending exceptions in that it's not necessarily a result of the specific call/parameters but instead the state of the runtime. For a pending exception there the case were we return an error, and we say that you may still need to make the call to check if there is a pending exception.

From the doc:

In cases where a return value other than napi_ok or napi_pending_exception is returned, napi_is_exception_pending must be called to check if an exception is pending. See the section on exceptions for more details.

Would the same apply to can_call_into_js ? ie if you get something other than napi_ok or the new exception for cannot call into js, you would need to have an API to check if that is the case? Or do we check it first so that regardless of what other error might be reported we'll report that you can't call into js if that is the case?

[gabrielschulhof]

@mhdawson

From what I understand so far it does share some similarities with pending exceptions in that it's not necessarily a result of the specific call/parameters but instead the state of the runtime. For a pending exception there the case were we return an error, and we say that you may still need to make the call to check if there is a pending exception.

In retrospect, I think that is a bug too. In many places we return a napi_generic_error when we should be returning napi_pending_exception. That would save people from having to make a second N-API call to see if an exception is pending.

To avoid this pattern in the future we need a flavour of our CHECK_* macros to be used in NAPI_PREAMBLE apis. They would perform the same check as the existing macros, but would return napi_pending_exception if the maybe is empty because an exception occurred in JS rather than for some other reason. I have asked that @himself65 start this pattern for napi_get_all_property_names and I am expanding on it in the type-tagging PR

From the doc:

In cases where a return value other than napi_ok or napi_pending_exception is returned, napi_is_exception_pending must be called to check if an exception is pending. See the section on exceptions for more details.

Would the same apply to can_call_into_js ? ie if you get something other than napi_ok or the new exception for cannot call into js, you would need to have an API to check if that is the case? Or do we check it first so that regardless of what other error might be reported we'll report that you can't call into js if that is the case?

Currently we are wasting our awareness of the fact that an exception is pending by returning a status other than napi_exception_pending in response to an empty maybe. napi_is_exception_pending() is sort of a catch-all in case the return status was not processed correctly, and so the awareness is maintained across N-API calls.

I don't believe we should introduce two instances of this pattern by making the case of being unable to run JS behave the same way, especially since the inability to run JS is not a state from which we can recover. There's no napi_get_and_clear_last_exception() that will restore the engine's ability to execute JS.

IMO the pattern we should promote is this:

// No preamble
napi_status napi_do_something(napi_env env, ...) {
  CHECK_ENV(env);
  ...
  CHECK_TO_OBJECT(...);
  ...
  RETURN_STATUS_IF_FALSE(..., napi_something_not_ok);
  ...
  return napi_clear_last_error(env);
}

// With preamble
napi_status napi_do_something_with_preamble(napi_env env, ...) {
  NAPI_PREAMBLE(env);
  ...
  CHECK_MAYBE_EMPTY_WITH_PREAMBLE(...);
  ...
  RETURN_STATUS_IF_FALSE_WITH_PREAMBLE(..., napi_something_not_ok);
  ...
  return GET_RETURN_STATUS(env);
}

where in the former case, the last error is set from the status passed into the macro and the status is returned, and in the latter case the last error is set from the status passed into the macro and returned, but only if no exception was caught, otherwise napi_pending_exception is set as last error and returned. This is OK, because whatever the status coming into the macro from the implementation of the N-API, the root cause is that an exception happened in JS, so that is the more relevant error. If no exception happened on the JS side, then the macro behaves exactly like its non-_WITH_PREAMBLE counterpart.

napi_can_run_js is relevant only in conjunction with NAPI_PREAMBLE, so NAPI_PREAMBLE() and, if the _WITH_PREAMBLE() variants land, its family of macros, need to be modified to not only account for an exception that may have happened, but also for the engine's inability to execute JS when they override the non-ok status passed in.

[mhdawson]

In retrospect, I think that is a bug too. In many places we return a napi_generic_error when we should be returning napi_pending_exception. That would save people from having to make a second N-API call to see if an exception is pending.

I think we specifically chose to make the pattern to only return pending exception if there was no other specific error and that is how it is documented up front. We might change that but we'd need to change the documentation as well and then we'd have the problem that sometimes you need to check and sometimes you don't

[mhdawson]

To add to the discussion we had talked about wether to return the pending exception or the more other return code and chose the other return code as otherwise there would be no way to find out about the other error. For example if you can somehow clear the error and continue (however unlikely that is) how would you know that the call also failed because you passed in invalid parameters.

[mhdawson]

One more question is we consider it a breaking change to reverse the earlier decision, ie give priority to pending_exception over other errors versus the original decision to do it the other way around?

[jasnell]

Ping @mhdawson @gabrielschulhof ... was this ever resolved?

[mhdawson]

@gabrielschulhof I think we ended up agreeing to try to make the code more consistent with the original intention versus introducing the new status. Is that what you remember ?

[gabrielschulhof]

@jasnell I think the state wherein the native side has no ability to execute JS is a genuinely separate state from all others and therefore it warrants its own status. The problem is that there are places where we should be returning such a status code, but we're returning a different one. Since we're ABI-stable we can't change those places to return this status code. That's why I marked the issue as n-api-semver-major.

[mhdawson]

@gabrielschulhof but I don't think we are going to make n-api-semver-major changes any time soon if at all, right? Therefore we should close as "won't do" or come up with a solution that is not n-api-semver-major.

[gabrielschulhof]

@mhdawson that's actually not a bad idea. The issue will remain tagged even if it is closed, and will not inflate the open issue count unnecessarily.

[kjvalencik]

Is there a way to check can_call_into_js directly in Node-API? The best I can tell, there is not. I am able to observe examples where napi_async_complete_callback gets a napi_ok status, but by the time other Node-API calls are made, can_call_into_js is no longer true.

The only way I see to check is to make a bogus call that uses NAPI_PREAMBLE as a side effect. E.g.,

// If this is `napi_invalid_arg`, everything is okay, otherwise there's either an exception or `can_call_into_js` is `false`
napi_typeof(env, NULL, NULL)

[gabrielschulhof]

@kjvalencik I think you can do this:

status = napi_do_something(env, ...);
if (status == napi_pending_exception) {
  boolean is_exception_pending = false;
  status = napi_is_exception_pending(env, &is_exception_pending);
  if (status == napi_ok) {
    if (is_exception_pending) {
      // an exception really is pending.
    } else {
      // cannot call into JS.
    }
  } else {
    // Something else failed.
  }
}

[gabrielschulhof]

Implemented in #47986.