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

Exceptions thrown in Init don't make it to JavaScript #239

Closed
fluggo opened this issue Mar 17, 2018 · 6 comments
Closed

Exceptions thrown in Init don't make it to JavaScript #239

fluggo opened this issue Mar 17, 2018 · 6 comments

Comments

@fluggo
Copy link

fluggo commented Mar 17, 2018

I suspect the problem is with the underlying NAPI than this project, because when I trace the code, it looks like node-addon-api does everything it's supposed to do. But this is the API where I found the problem, so I'm reporting it here.

test.cpp:

#include "napi.h"
#include <stdexcept>
#include <iostream>

Napi::Object Init(Napi::Env env, Napi::Object exports) {
  try {
    throw Napi::Error::New(env, "Test");
    return exports;
  }
  catch(const Napi::Error& e) {
    std::cout << "Caught NAPI exception\n";
    throw;
  }
  catch(const std::exception& e) {
    std::cout << "Caught std exception\n";
    throw;
  }
  catch(...) {
    std::cout << "Caught exception\n";
    throw;
  }
}

NODE_API_MODULE(test, Init)

binding.gyp:

{
  "targets": [
    {
      "target_name": "test",
      "sources": ["test.cpp"],
      "cflags!": ["-fno-exceptions"],
      "cflags": ["-std=c++11"],
      "cflags_cc!": ["-fno-exceptions"],
      "include_dirs": [
        "<!@(node -p \"require('node-addon-api').include\")"
      ],
      "dependencies": [
        "<!(node -p \"require('node-addon-api').gyp\")"
      ]
    }
  ]
}

index.js:

const test = require('./build/Debug/test.node');
console.log('No exception thrown');

Output:

~/software/napi-init-exception-test $ node -v
v8.9.4
~/software/napi-init-exception-test $ node index.js
Caught NAPI exception
No exception thrown
(node:1243) Warning: N-API is an experimental feature and could change at any time.
@fluggo
Copy link
Author

fluggo commented Mar 19, 2018

It looks like this may be a bug in NAPI itself. Running a modified version of NAPI's hello world which throws an exception after adding the hello method to exports, the exception gets reported on the hello() call:

/app/pure-napi/hello.js:3
console.log(addon.hello()); // 'world'
                  ^

Error [CODE]: Test error
    at Object.Module._extensions..node (module.js:678:18)
    at Module.load (module.js:561:32)
    at tryModuleLoad (module.js:501:12)
    at Function.Module._load (module.js:493:3)
    at Module.require (module.js:593:17)
    at require (internal/module.js:11:18)
    at bindings (/app/pure-napi/node_modules/bindings/bindings.js:76:44)
    at Object.<anonymous> (/app/pure-napi/hello.js:1:94)
    at Module._compile (module.js:649:30)
    at Object.Module._extensions..js (module.js:660:10)

Removing the call to hello causes no exception to be reported at all. Doing the same with node-addon-api produces the same behavior.

@mhdawson
Copy link
Member

Any chance you've confirmed it has the correct behaviour with a non n-api addon ?

@fluggo
Copy link
Author

fluggo commented Mar 20, 2018

Verified, it does work when not using N-API. In a pure V8-only addon:

void Init(Handle<Object> exports) {
  Isolate* isolate = Isolate::GetCurrent();
  exports->Set(String::NewFromUtf8(isolate, "hello"),
      FunctionTemplate::New(isolate, Method)->GetFunction());
  isolate->ThrowException(String::NewFromUtf8(isolate, "Test error"));
}

I get:

root@a5d5c8becf83:/app/pure-node# node hello.js

module.js:678
  return process.dlopen(module, path.toNamespacedPath(filename));
                 ^
Test error

@fluggo
Copy link
Author

fluggo commented Mar 20, 2018

Also, I did put this on the main node tracker fixed link nodejs/node#19437.

@mhdawson
Copy link
Member

@fluggo thanks for validating, will help narrow down where to start looking.

@mhdawson
Copy link
Member

mhdawson commented Apr 2, 2018

Believe this was fixed in nodejs/node#19437. Please re-open if that is not the case. @fluggo thanks for reporting.

@mhdawson mhdawson closed this as completed Apr 2, 2018
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

No branches or pull requests

2 participants