Skip to content

Commit

Permalink
Add support for Error::Fatal
Browse files Browse the repository at this point in the history
* Added static method `Error::Fatal` to invoke `napi_fatal_error`
* Added `node_internals.cc/h` to shim missing internal functions
* Added a test for `Error::Fatal`
* Replaced usage of assert with calls to `Error::Fatal`

PR-URL: #70
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
  • Loading branch information
kfarnung authored and mhdawson committed Jul 14, 2017
1 parent 179d135 commit 28fad43
Show file tree
Hide file tree
Showing 8 changed files with 65 additions and 19 deletions.
30 changes: 19 additions & 11 deletions napi-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

// Note: Do not include this file directly! Include "napi.h" instead.

#include <cassert>
#include <cstring>

namespace Napi {
Expand Down Expand Up @@ -42,6 +41,13 @@ namespace details {

#endif // NAPI_CPP_EXCEPTIONS

#define NAPI_FATAL_IF_FAILED(status, location, message) \
do { \
if ((status) != napi_ok) { \
Error::Fatal((location), (message)); \
} \
} while (0)

// For use in JS to C++ callback wrappers to catch any Napi::Error exceptions
// and rethrow them as JavaScript exceptions before returning from the callback.
template <typename Callable>
Expand Down Expand Up @@ -1418,24 +1424,24 @@ inline Error Error::New(napi_env env) {

const napi_extended_error_info* info;
status = napi_get_last_error_info(env, &info);
assert(status == napi_ok);
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_get_last_error_info");

if (status == napi_ok) {
if (info->error_code == napi_pending_exception) {
status = napi_get_and_clear_last_exception(env, &error);
assert(status == napi_ok);
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_get_and_clear_last_exception");
}
else {
const char* error_message = info->error_message != nullptr ?
info->error_message : "Error in native callback";

bool isExceptionPending;
status = napi_is_exception_pending(env, &isExceptionPending);
assert(status == napi_ok);
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_is_exception_pending");

if (isExceptionPending) {
status = napi_get_and_clear_last_exception(env, &error);
assert(status == napi_ok);
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_get_and_clear_last_exception");
}

napi_value message;
Expand All @@ -1444,7 +1450,7 @@ inline Error Error::New(napi_env env) {
error_message,
std::strlen(error_message),
&message);
assert(status == napi_ok);
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_create_string_utf8");

if (status == napi_ok) {
switch (info->error_code) {
Expand All @@ -1458,7 +1464,7 @@ inline Error Error::New(napi_env env) {
status = napi_create_error(env, nullptr, message, &error);
break;
}
assert(status == napi_ok);
NAPI_FATAL_IF_FAILED(status, "Error::New", "napi_create_error");
}
}
}
Expand All @@ -1474,6 +1480,10 @@ inline Error Error::New(napi_env env, const std::string& message) {
return Error::New<Error>(env, message.c_str(), message.size(), napi_create_error);
}

inline NAPI_NO_RETURN void Error::Fatal(const char* location, const char* message) {
napi_fatal_error(location, message);
}

inline Error::Error() : ObjectReference(), _message(nullptr) {
}

Expand All @@ -1483,7 +1493,7 @@ inline Error::Error(napi_env env, napi_value value) : ObjectReference(env, nullp

// Avoid infinite recursion in the failure case.
// Don't try to construct & throw another Error instance.
assert(status == napi_ok);
NAPI_FATAL_IF_FAILED(status, "Error::Error", "napi_create_reference");
}
}

Expand Down Expand Up @@ -1661,9 +1671,7 @@ inline Reference<T>::Reference(const Reference<T>& other)
// Copying is a limited scenario (currently only used for Error object) and always creates a
// strong reference to the given value even if the incoming reference is weak.
napi_status status = napi_create_reference(_env, value, 1, &_ref);

// TODO - Switch to napi_fatal_error() once it exists.
assert(status == napi_ok);
NAPI_FATAL_IF_FAILED(status, "Reference<T>::Reference", "napi_create_reference");
}
}

Expand Down
2 changes: 2 additions & 0 deletions napi.h
Original file line number Diff line number Diff line change
Expand Up @@ -1067,6 +1067,8 @@ namespace Napi {
static Error New(napi_env env, const char* message);
static Error New(napi_env env, const std::string& message);

static NAPI_NO_RETURN void Fatal(const char* location, const char* message);

Error();
Error(napi_env env, napi_value value);

Expand Down
5 changes: 5 additions & 0 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -823,6 +823,11 @@ napi_status napi_get_last_error_info(napi_env env,
return napi_ok;
}

NAPI_NO_RETURN void napi_fatal_error(const char* location,
const char* message) {
node::FatalError(location, message);
}

napi_status napi_create_function(napi_env env,
const char* utf8name,
napi_callback cb,
Expand Down
9 changes: 9 additions & 0 deletions src/node_api.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@
# define NAPI_MODULE_EXPORT __attribute__((visibility("default")))
#endif

#ifdef __GNUC__
#define NAPI_NO_RETURN __attribute__((noreturn))
#else
#define NAPI_NO_RETURN
#endif


typedef void (*napi_addon_register_func)(napi_env env,
napi_value exports,
Expand Down Expand Up @@ -104,6 +110,9 @@ NAPI_EXTERN napi_status
napi_get_last_error_info(napi_env env,
const napi_extended_error_info** result);

NAPI_EXTERN NAPI_NO_RETURN void napi_fatal_error(const char* location,
const char* message);

// Getters for defined singletons
NAPI_EXTERN napi_status napi_get_undefined(napi_env env, napi_value* result);
NAPI_EXTERN napi_status napi_get_null(napi_env env, napi_value* result);
Expand Down
5 changes: 5 additions & 0 deletions test/error.cc
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,10 @@ void CatchAndRethrowErrorThatEscapesScope(const CallbackInfo& info) {

#endif // NAPI_CPP_EXCEPTIONS

void ThrowFatalError(const CallbackInfo& info) {
Error::Fatal("Error::ThrowFatalError", "This is a fatal error");
}

} // end anonymous namespace

Object InitError(Env env) {
Expand All @@ -169,5 +173,6 @@ Object InitError(Env env) {
exports["throwErrorThatEscapesScope"] = Function::New(env, ThrowErrorThatEscapesScope);
exports["catchAndRethrowErrorThatEscapesScope"] =
Function::New(env, CatchAndRethrowErrorThatEscapesScope);
exports["throwFatalError"] = Function::New(env, ThrowFatalError);
return exports;
}
20 changes: 17 additions & 3 deletions test/error.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,18 @@
const buildType = process.config.target_defaults.default_configuration;
const assert = require('assert');

test(require(`./build/${buildType}/binding.node`));
test(require(`./build/${buildType}/binding_noexcept.node`));
if (process.argv[2] === 'fatal') {
const binding = require(process.argv[3]);
binding.error.throwFatalError();
return;
}

test(`./build/${buildType}/binding.node`);
test(`./build/${buildType}/binding_noexcept.node`);

function test(bindingPath) {
const binding = require(bindingPath);

function test(binding) {
assert.throws(() => binding.error.throwApiError('test'), err => {
return err instanceof Error && err.message.includes('Invalid');
});
Expand Down Expand Up @@ -56,4 +64,10 @@ function test(binding) {
assert.throws(() => binding.error.catchAndRethrowErrorThatEscapesScope('test'), err => {
return err instanceof Error && err.message === 'test' && err.caught;
});

const p = require('./napi_child').spawnSync(
process.execPath, [ __filename, 'fatal', bindingPath ]);
assert.ifError(p.error);
assert.ok(p.stderr.toString().includes(
'FATAL ERROR: Error::ThrowFatalError This is a fatal error'));
}
6 changes: 1 addition & 5 deletions test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,7 @@ if (typeof global.gc === 'function') {
console.log('\nAll tests passed!');
} else {
// Make it easier to run with the correct (version-dependent) command-line args.
const args = [ '--expose-gc', __filename ];
if (require('../index').isNodeApiBuiltin) {
args.splice(0, 0, '--napi-modules');
}
const child = require('child_process').spawnSync(process.argv[0], args, {
const child = require('./napi_child').spawnSync(process.argv[0], [ '--expose-gc', __filename ], {
stdio: 'inherit',
});
process.exitCode = child.status;
Expand Down
7 changes: 7 additions & 0 deletions test/napi_child.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
// Makes sure that child processes are spawned appropriately.
exports.spawnSync = function(command, args, options) {
if (require('../index').isNodeApiBuiltin) {
args.splice(0, 0, '--napi-modules');
}
return require('child_process').spawnSync(command, args, options);
};

0 comments on commit 28fad43

Please sign in to comment.