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

don't try to escape null #245

Closed
wants to merge 1 commit into from
Closed

Conversation

mhdawson
Copy link
Member

@mhdawson mhdawson commented Apr 3, 2018

If an exception is pending from Call Or MakeCallback on
FunctionReference the return value may be nullptr.
Check for a pending exception and don't try to escape in these cases.

Refs: #233

Next step after this would be to add support for throwing fatal exception with node-addon-api which can include a test that validates the exception is correct.

If an exception is pending from Call Or MakeCallback on
FunctionReference the return value may be nullptr.
Check for a pending exception and don't try to escape in these cases.
@mhdawson mhdawson mentioned this pull request Apr 3, 2018
@digitalinfinity
Copy link
Contributor

Might be a dumb question but I thought the general model in node-addon-api is to have script exceptions propagate as C++ exceptions (i.e we'd thrown an exception)- so why doesn't Call or MakeCallback throw if an exception's pending?

@mhdawson
Copy link
Member Author

mhdawson commented Apr 4, 2018

@digitalinfinity I don't quite get your question in the context of this PR. The problem that it addresses is that we invoked Call or MakeCallback and the method that was called threw an exception.

@mhdawson
Copy link
Member Author

mhdawson commented Apr 4, 2018

If you wonder why that did not use C++ node-addon-api supports both using C++ exceptions and not using them.

@digitalinfinity
Copy link
Contributor

Right, I understand that the script method that was called threw an exception- my question was more along the lines of, why not propagate that exception as a C++ exception? I guess your change only applies to the non-exception cases?

@mhdawson
Copy link
Member Author

mhdawson commented Apr 5, 2018

It applies to the default case where the test case was failing. The problem was that it tries to unwrap then it throws a different exception so you get the wrong exception being returned. That would likely be a problem in both cases.

@mhdawson
Copy link
Member Author

mhdawson commented Apr 9, 2018

@digitalinfinity @gabrielschulhof can you review?

mhdawson added a commit that referenced this pull request Apr 10, 2018
If an exception is pending from Call Or MakeCallback on
FunctionReference the return value may be nullptr.
Check for a pending exception and don't try to escape in these cases.

PR-URL: #245
Refs: #233
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
@mhdawson
Copy link
Member Author

Landed as cf6c93e

@mhdawson mhdawson closed this Apr 10, 2018
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
If an exception is pending from Call Or MakeCallback on
FunctionReference the return value may be nullptr.
Check for a pending exception and don't try to escape in these cases.

PR-URL: nodejs/node-addon-api#245
Refs: nodejs/node-addon-api#233
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
If an exception is pending from Call Or MakeCallback on
FunctionReference the return value may be nullptr.
Check for a pending exception and don't try to escape in these cases.

PR-URL: nodejs/node-addon-api#245
Refs: nodejs/node-addon-api#233
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
If an exception is pending from Call Or MakeCallback on
FunctionReference the return value may be nullptr.
Check for a pending exception and don't try to escape in these cases.

PR-URL: nodejs/node-addon-api#245
Refs: nodejs/node-addon-api#233
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
austinli64 added a commit to austinli64/node-addon-api that referenced this pull request May 9, 2023
If an exception is pending from Call Or MakeCallback on
FunctionReference the return value may be nullptr.
Check for a pending exception and don't try to escape in these cases.

PR-URL: nodejs/node-addon-api#245
Refs: nodejs/node-addon-api#233
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
If an exception is pending from Call Or MakeCallback on
FunctionReference the return value may be nullptr.
Check for a pending exception and don't try to escape in these cases.

PR-URL: nodejs/node-addon-api#245
Refs: nodejs/node-addon-api#233
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants