Skip to content
This repository has been archived by the owner on Jun 18, 2021. It is now read-only.

errors caught from n-api addons shouldn't trigger UncaughtException reports #133

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

richardlau
Copy link
Member

WIP.

Refs: #131

Added test. Need to look at how to fix.

@richardlau
Copy link
Member Author

richardlau commented Jun 26, 2019

I think I'm going to need some help here. I've added two tests to this PR, one based on n-api and the other using nan which I think should replicate #131. However the nan version is passing while the n-api version is not. I expected both tests to fail.

Could someone:

  1. Verify that the two tests (the nan and n-api versions) are equivalent?
  2. Explain the difference in behaviour (i.e. why the n-api version results in an UncaughtException but the nan version does not)?

cc @nodejs/addon-api @nodejs/n-api @mmarchini

@bnoordhuis
Copy link
Member

Verify that the two tests (the nan and n-api versions) are equivalent?

They look equivalent to me. Ultimately they both end up calling v8::Isolate::ThrowException().

@richardlau
Copy link
Member Author

Thanks Ben. So the question remains over the behavioral differences and whether this is an n-api issue (since it doesn't happen with the nan based equivalent)?

@mmarchini
Copy link

N-API seems to create a v8::TryCatch object before throwing (as part of the NAPI_PREAMBLE). It might be related, depending on how V8's catch-prediction handles C++ TryCatch blocks (although I couldn't replicate the same behavior in nan, even after wrapping ThrowError with a v8::TryCatch object).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants