Node-addon-api: Error messages and MakeCallback

[fivdi]

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]

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

[mhdawson]

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]

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 https://github.com/nodejs/node/issues/15371

[mhdawson]

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]

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

[mhdawson]

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 https://github.com/nodejs/node/pull/19362 for the PR to node core to fix.

[mhdawson]

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

[fivdi]

Nice :smile: