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

Error messages and MakeCallback #235

Closed
fivdi opened this issue Mar 9, 2018 · 8 comments
Closed

Error messages and MakeCallback #235

fivdi opened this issue Mar 9, 2018 · 8 comments

Comments

@fivdi
Copy link

fivdi commented Mar 9, 2018

Lets say we have the below addon where RunCallbackWithCall uses Call to call a callback and RunCallbackWithMakeCallback uses MakeCallback to call a callback.

#include <napi.h>

void RunCallbackWithCall(const Napi::CallbackInfo& info) {
  Napi::Env env = info.Env();
  Napi::Function cb = info[0].As<Napi::Function>();
  cb.Call({ Napi::String::New(env, "Hello, World!") });
}

void RunCallbackWithMakeCallback(const Napi::CallbackInfo& info) {
  Napi::Env env = info.Env();
  Napi::Function cb = info[0].As<Napi::Function>();
  cb.MakeCallback(env.Global(), { Napi::String::New(env, "Hello, World!") });
}

Napi::Object Init(Napi::Env env, Napi::Object exports) {
  exports["runCallbackWithCall"] =
    Napi::Function::New(env, RunCallbackWithCall);
  exports["runCallbackWithMakeCallback"] =
    Napi::Function::New(env, RunCallbackWithMakeCallback);

  return exports;
}

NODE_API_MODULE(hello, Init)

If the callback passed to runCallbackWithCall throws an Error like this:

const addon = require('bindings')('addon.node');

addon.runCallbackWithCall((str) => {
  console.log(str);
  throw new Error('Error in callback');
});

Then the following is displayed:

Hello, World!
/home/pi/node-addon-api-experiments2/test/run-callback-with-call.js:5
addon.runCallbackWithCall((str) => {
      ^

Error: Error in callback
    at addon.runCallbackWithCall (/home/pi/node-addon-api-experiments2/test/run-callback-with-call.js:7:9)
    at Object.<anonymous> (/home/pi/node-addon-api-experiments2/test/run-callback-with-call.js:5:7)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Function.Module.runMain (module.js:693:10)
    at startup (bootstrap_node.js:188:16)
    at bootstrap_node.js:609:3

Everything looks ok here. As is to be expected "Error: Error in callback" is displayed.

However, If the callback passed to runCallbackWithMakeCallback throws an Error like this:

const addon = require('bindings')('addon.node');

addon.runCallbackWithMakeCallback((str) => {
  console.log(str);
  throw new Error('Error in callback');
});

Then the following is displayed:

Hello, World!
/home/pi/node-addon-api-experiments2/test/run-callback-with-makecallback.js:5
addon.runCallbackWithMakeCallback((str) => {
      ^

Error: Unknown failure
    at Object.<anonymous> (/home/pi/node-addon-api-experiments2/test/run-callback-with-makecallback.js:5:7)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)
    at tryModuleLoad (module.js:505:12)
    at Function.Module._load (module.js:497:3)
    at Function.Module.runMain (module.js:693:10)
    at startup (bootstrap_node.js:188:16)
    at bootstrap_node.js:609:3

Things don't look correct here. I would expect "Error: Error in callback" to be displayed but "Error: Unknown failure" is displayed.

@mhdawson
Copy link
Member

Can youu attach a zip with the all of the files so that I can easily run/recreate?

@mhdawson
Copy link
Member

Error: Unknown failure is one of the messages used within the C n-api so either we are failing to preserve the original error message in the n-api call, or a later call to n-api is made while the exception is pending.

Travelling today but will try to investigate further later this week. If you can give me a zip I can just unzip and run to recreate that will help get me started faster.

@NickNaso
Copy link
Member

Hi everyone,
I tried to recreate the same example proposed by @fivdi and I got the same behaviour. I post my project in the zip file hope to be helpful.
example-make-callback.zip
I don't know if this is correlated with this issue nodejs/node#15371

@mhdawson
Copy link
Member

mhdawson commented Mar 14, 2018

I investigated on the plane today:

  • The c n-api is ok and we have good test coverage which validates it works ok on that side
  • In node-addon-api it looks like we are overwriting the exception incorrectly and it should be an easy fix.

@NickNaso thanks, I'll use your files to validate the fix.

I believe it is unrelated to nodejs/node#15371

@mhdawson
Copy link
Member

Seems to be a bit more subtle than I thought, investigating further.

mhdawson added a commit to mhdawson/io.js that referenced this issue Mar 15, 2018
Add checks for a pending exception in napi_make_callback
after the callback has been invoked.  If there is a pending
exception then we need to avoid checking the result as that
will not be able to complete properly.

Add additional checks to the unit test for napi_make_callback
to catch this case.

Fixes: nodejs/node-addon-api#235
@mhdawson
Copy link
Member

mhdawson commented Mar 15, 2018

Ok so there was a combination of factors here:

  1. a bug in napi_make_callback. The issue is that it was not checking to see if there was an exception pending after the callback, doing another call which set an error code so we ending up with the original pending exception but an error code other than napi_pending_exception.
  2. The existing unit test for napi_make_callback did not catch the problem because although a new error code had been set, the original exception was still pending and there was no check to validate the error code expected. In this case it was just validating that the expected exception was pending on return.
  3. node-addon-api gives priority to errors if there is both an exception pending and an error. So in the test using node-addon-api it removed the original pending exception and threw a new one reflecting the new error code that had been returned. I think think this is generally ok as we don't expect an error and a pending exception and if there are both we need to choose one to percolate up in an exception.

See nodejs/node#19362 for the PR to node core to fix.

@mhdawson
Copy link
Member

I'll have to take another look at nodejs/node#15371 as well since I'm not sure its unrelated after all.

@fivdi
Copy link
Author

fivdi commented Mar 16, 2018

Nice 😄

targos pushed a commit to nodejs/node that referenced this issue Mar 17, 2018
Add checks for a pending exception in napi_make_callback
after the callback has been invoked.  If there is a pending
exception then we need to avoid checking the result as that
will not be able to complete properly.

Add additional checks to the unit test for napi_make_callback
to catch this case.

PR-URL: #19362
Fixes: nodejs/node-addon-api#235
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Mar 20, 2018
Add checks for a pending exception in napi_make_callback
after the callback has been invoked.  If there is a pending
exception then we need to avoid checking the result as that
will not be able to complete properly.

Add additional checks to the unit test for napi_make_callback
to catch this case.

PR-URL: #19362
Fixes: nodejs/node-addon-api#235
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Mar 20, 2018
Add checks for a pending exception in napi_make_callback
after the callback has been invoked.  If there is a pending
exception then we need to avoid checking the result as that
will not be able to complete properly.

Add additional checks to the unit test for napi_make_callback
to catch this case.

PR-URL: #19362
Fixes: nodejs/node-addon-api#235
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Apr 12, 2018
Add checks for a pending exception in napi_make_callback
after the callback has been invoked.  If there is a pending
exception then we need to avoid checking the result as that
will not be able to complete properly.

Add additional checks to the unit test for napi_make_callback
to catch this case.

PR-URL: nodejs#19362
Fixes: nodejs/node-addon-api#235
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this issue Apr 16, 2018
Add checks for a pending exception in napi_make_callback
after the callback has been invoked.  If there is a pending
exception then we need to avoid checking the result as that
will not be able to complete properly.

Add additional checks to the unit test for napi_make_callback
to catch this case.

PR-URL: nodejs#19362
Fixes: nodejs/node-addon-api#235
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue Apr 16, 2018
Add checks for a pending exception in napi_make_callback
after the callback has been invoked.  If there is a pending
exception then we need to avoid checking the result as that
will not be able to complete properly.

Add additional checks to the unit test for napi_make_callback
to catch this case.

Backport-PR-URL: #19447
PR-URL: #19362
Fixes: nodejs/node-addon-api#235
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this issue May 1, 2018
Add checks for a pending exception in napi_make_callback
after the callback has been invoked.  If there is a pending
exception then we need to avoid checking the result as that
will not be able to complete properly.

Add additional checks to the unit test for napi_make_callback
to catch this case.

Backport-PR-URL: #19265
PR-URL: #19362
Fixes: nodejs/node-addon-api#235
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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 a pull request may close this issue.

3 participants