Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

n-api error status discussion #29849

Closed
legendecas opened this issue Oct 5, 2019 · 9 comments
Closed

n-api error status discussion #29849

legendecas opened this issue Oct 5, 2019 · 9 comments
Labels
node-api Issues and PRs related to the Node-API.

Comments

@legendecas
Copy link
Member

  • Version: master
  • Platform: -
  • Subsystem: n-api

We could find out that there are several napi_status existing and been used in various n-api functions.

  • napi_ok:
    For any n-api call that is successful.

  • napi_cancelled
    For canceled async works.

  • napi_escape_called_twice
  • napi_handle_scope_mismatch
  • napi_callback_scope_mismatch
    For handle scope error conditions.

  • napi_queue_full
  • napi_closing
    For ThreadSafeFunction error conditions.

  • napi_invalid_arg
    This is what I'd like to discuss about. If I got it right, mostly, invalid args of calling of n-api should get into this status, except following type exceptions (mostly JavaScript primitive values, but array and date are not, name is a virtual type that is defined in ECMA spec):

  • napi_object_expected

  • napi_string_expected

  • napi_name_expected

  • napi_function_expected

    • Exceptions we have now:
    • napi_instanceof: would throw JavaScript TypeError if constructor is not a function, and return a napi_function_expected status.
  • napi_number_expected

  • napi_boolean_expected

  • napi_array_expected

  • napi_bigint_expected

  • napi_date_expected


Except for all above statuses, following two is additional generic error status:

  • napi_generic_failure
    This could be used in most places IMO, but it has been mixed up with napi_pending_exception in some conditions:

  • napi_pending_exception
    For any n-api call that has pending JavaScript Exception. If I understand n-api document right, every non-napi_ok status should throw a JavaScript exception, and napi_pending_exception should be used if the previous JavaScript exception was not handled while calling sequent n-apis.

The return value will be napi_ok if the request was successful and no uncaught JavaScript exception was thrown. If an error occurred AND an exception was thrown, the napi_status value for the error will be returned. If an exception was thrown, and no error occurred, napi_pending_exception will be returned. Link


So here comes the unresolved questions:

  • How do we define new type expectation status constraints: should they be limited to JavaScript primitive values? Or any non-primitive types that have an equivalent napi_is_<typename> function?
  • In which conditions N-APIs should throw JavaScript exceptions? and for those already throwing, which status should be returned? napi_generic_failure or napi_pending_exception?

Refs: #29768 (comment)
Related: #29847

/cc @gabrielschulhof @mhdawson

@mhdawson
Copy link
Member

mhdawson commented Oct 7, 2019

If there is an exception pending (whether it was just thrown or not) but no other error then napi_pending_exception should be returned.

If there is a pending exception it is still ok to return a more specific error. The docs say that in addition to handling that error you must still check for a pending exception.

Generally I think we should avoid N-API itself throwing an exception and instead have it return an error. So for:

In which conditions N-APIs should throw JavaScript exceptions? and for those already throwing, which status should be returned? napi_generic_failure or napi_pending_exception?

Either is actually legal. I'd have to look at the specific cases to see if one made more sense than another but my take would be napi_generic_failure as N-API should generally be returning an error as opposed to throwing an exception and returning an error is at least a bit more consistent with that.

@gabrielschulhof
Copy link
Contributor

@legendecas ideally

  • N-API would throw no exceptions of its own.
  • If, in the course of interacting with the engine, an exception was thrown, it would return napi_pending_exception.
  • Otherwise, if an error condition arose because one of the checks performed by N-API failed, then it would return an appropriate non-napi_ok status.

Unfortunately, we have not been very consistent in maintaining this principle as we've moved N-API forward 😕

For example, in napi_get_property() we have

CHECK_MAYBE_EMPTY(env, get_maybe, napi_generic_failure);

Before returning napi_generic_failure we should check whether an exception was thrown and return napi_pending_exception.

We have quite a few inconsistencies like these in the implementation.

@legendecas
Copy link
Member Author

legendecas commented Oct 9, 2019

So for question 2:

In which conditions N-APIs should throw JavaScript exceptions? and for those already throwing, which status should be returned? napi_generic_failure or napi_pending_exception?

Could we get into a summary that throwing in N-API shall be prevented from and preferring returning an error napi_status?

For condition of which status shall be returned depends on the case specifically. Likewise, napi_pending_exception shall be preferred if the exception was thrown by the engine. And napi_generic_failure shall be considered in most conditions.

On the statement above, napi_status enum might get appended every time we have a new error condition. E.g. in #29768 two new napi_status were added. Though I am not much uncomfortable with a big enum of napi_status. Would it be concerning anyone?

@gabrielschulhof
Copy link
Contributor

@legendecas that sounds like a good summary 👍 I don't think there's any problem with a large enum.

@mhdawson
Copy link
Member

mhdawson commented Oct 9, 2019

I'm not worried about too big an enum either, although I'm not sure it will get all that big anyway.

@legendecas
Copy link
Member Author

legendecas commented Oct 10, 2019

Just reviewing the error handling in node-addon-api. I noticed that the thrown error in N-APIs with non-napi_pending_exception would be ignored in https://github.com/nodejs/node-addon-api/blob/master/napi-inl.h#L2006-L2013 (the error would be overriden in https://github.com/nodejs/node-addon-api/blob/master/napi-inl.h#L2023-L2032).

So I updated #29847 to correct the related parts. PTAL :)

@gabrielschulhof
Copy link
Contributor

@legendecas that sounds like a good summary 👍 I don't think there's any problem with a large enum.

So for question 2:

In which conditions N-APIs should throw JavaScript exceptions? and for those already throwing, which status should be returned? napi_generic_failure or napi_pending_exception?

Could we get into a summary that throwing in N-API shall be prevented from and preferring returning an error napi_status?

Absolutely!

For condition of which status shall be returned depends on the case specifically. Likewise, napi_pending_exception shall be preferred if the exception was thrown by the engine. And napi_generic_failure shall be considered in most conditions.

On the statement above, napi_status enum might get appended every time we have a new error condition. E.g. in #29768 two new napi_status were added. Though I am not much uncomfortable with a big enum of napi_status. Would it be concerning anyone?

I don't believe a big enum should be a concern. We have many error scenarios and we should be able to distinguish between them.

The only constraint is that whatever we have today, and however inconsistent it is, must remain so, because we must not introduce breaking changes.

@richardlau richardlau added the node-api Issues and PRs related to the Node-API. label Oct 18, 2019
Trott pushed a commit that referenced this issue Jan 14, 2020
N-API statuses shall be preferred over throwing JavaScript Errors on
checks occurred in N-APIs.

PR-URL: #31312
Refs: #29849
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 16, 2020

Unverified

The email in this signature doesn’t match the committer email.
N-API statuses shall be preferred over throwing JavaScript Errors on
checks occurred in N-APIs.

PR-URL: #31312
Refs: #29849
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
codebytere pushed a commit that referenced this issue Mar 14, 2020

Verified

This commit was signed with the committer’s verified signature.
codebytere Shelley Vohr
N-API statuses shall be preferred over throwing JavaScript Errors on
checks occurred in N-APIs.

PR-URL: #31312
Refs: #29849
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
codebytere pushed a commit that referenced this issue Mar 17, 2020

Verified

This commit was signed with the committer’s verified signature.
codebytere Shelley Vohr
N-API statuses shall be preferred over throwing JavaScript Errors on
checks occurred in N-APIs.

PR-URL: #31312
Refs: #29849
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@targos
Copy link
Member

targos commented Sep 28, 2020

@legendecas what's the status of this issue?

@legendecas
Copy link
Member Author

This PR has no further action to take. Closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

No branches or pull requests

5 participants