Error messages and MakeCallback

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.

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

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.

@fivdi

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

@NickNaso

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

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

n-api: add missing exception checking

Ok so there was a combination of factors here:

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.
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.
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.

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

Nice 😄

n-api: add missing exception checking

Navigation Menu

Repository navigation

Error messages and MakeCallback #235

4 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Error messages and MakeCallback #235

Description

Activity

mhdawson commented on Mar 14, 2018

mhdawson commented on Mar 14, 2018

NickNaso commented on Mar 14, 2018

mhdawson commented on Mar 14, 2018

mhdawson commented on Mar 14, 2018

mhdawson commented on Mar 15, 2018

mhdawson commented on Mar 15, 2018

fivdi commented on Mar 16, 2018

4 remaining items

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions