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

Add support for Error::Fatal #70

Closed
wants to merge 1 commit into from

Conversation

kfarnung
Copy link
Contributor

  • 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

@kfarnung
Copy link
Contributor Author

/cc @digitalinfinity @jasongin @gabrielschulhof

Ignore the changes to node_api.cc/h for now as they include the in-progress changes from nodejs/node#13971. I also need to use napi_fatal_error in a few locations throughout the wrapper code, but haven't gotten there yet.

test/index.js Outdated
@@ -28,7 +28,7 @@ if (typeof global.gc === 'function') {
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], args, {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you intend to delete the splicing lines above?


#ifndef SRC_NODE_INTERNALS_H_
#define SRC_NODE_INTERNALS_H_

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put a comment in here explaining that this is not the real, complete "node_internals", but just a shim for building node_api.cc outside of node?

napi-inl.h Outdated
@@ -42,6 +41,9 @@ namespace details {

#endif // NAPI_CPP_EXCEPTIONS

#define NAPI_FATAL_IF_FAILED(status, location, message) \
if ((status) != napi_ok) Error::Fatal((location), (message));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: wrap this in a do { ... } while so that if (...) NAPI_FATAL_IF_FAILED(...); else ... does the right thing.

src/node_api.cc Outdated
v8::ObjectTemplate::New(isolate);
wrapperTemplate->SetInternalFieldCount(1);
v8::Local<v8::Object> wrapper =
wrapperTemplate->NewInstance(context).ToLocalChecked();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: indent line continuations by four spaces, not two.

src/node_api.cc Outdated

obj->SetInternalField(0, v8::External::New(isolate, native_object));
// Create a wrapper object with an internal field to hold the wrapped pointer.
v8::Local<v8::ObjectTemplate> wrapperTemplate =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: wrapper_template (snake_case, not camelCase, for locals.)

Substance: creating an ObjectTemplate every time leaks memory. V8 caches them internally and it doesn't put an upper bound or a lifetime on elements in the cache. Create a single ObjectTemplate and reuse that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm ... we do this upstream as well.

src/node_api.cc Outdated
// Insert the wrapper into the object's prototype chain.
v8::Local<v8::Value> proto = obj->GetPrototype();
wrapper->SetPrototype(proto);
obj->SetPrototype(wrapper);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use the Maybe overloads here?

@@ -0,0 +1,90 @@
// Copyright Joyent, Inc. and other Node contributors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of the code in this file is (c) Joyent, it's all (c) yours truly. You can leave out the copyright boilerplate or reduce it to just 'Node contributors.'

The same applies to the preceding file, I think: it's a mix of code authored by @mscdex and me (and maybe @indutny.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bnoordhuis, I was looking for some guidance here. I'll change the copyright headers.

test/error.js Outdated
process.execPath, [ __filename, 'fatal', bindingPath ]);
assert.ifError(p.error);
assert.ok(p.stderr.toString().includes(
'FATAL ERROR: Error::ThrowFatalError This is a fatal error'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: 4 space indent for line continuations.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Jun 30, 2017
V8 caches and does not subsequently release `ObjectTemplate` instances.
Thus, we need to store the `ObjectTemplate` from which we derive object
instances we use for `napi_wrap()` and function/accessor context in a
persistent in the `napi_env`.

nodejs/node-addon-api#70 (comment)
mhdawson pushed a commit to nodejs/node that referenced this pull request Jul 6, 2017
V8 caches and does not subsequently release `ObjectTemplate` instances.
Thus, we need to store the `ObjectTemplate` from which we derive object
instances we use for `napi_wrap()` and function/accessor context in a
persistent in the `napi_env`.

nodejs/node-addon-api#70 (comment)

PR-URL: #13999
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
addaleax pushed a commit to nodejs/node that referenced this pull request Jul 11, 2017
V8 caches and does not subsequently release `ObjectTemplate` instances.
Thus, we need to store the `ObjectTemplate` from which we derive object
instances we use for `napi_wrap()` and function/accessor context in a
persistent in the `napi_env`.

nodejs/node-addon-api#70 (comment)

PR-URL: #13999
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
@jasongin
Copy link
Member

@kfarnung Is this ready to merge? It looks like you've addressed @bnoordhuis's feedback.

@kfarnung
Copy link
Contributor Author

I was waiting on the upstream change (nodejs/node#13971) to land before trying to get this merged.

@mhdawson
Copy link
Member

I guess this now needs to be rebased to remove the changes to the node files.

@kfarnung
Copy link
Contributor Author

Yup, I'll take care of that momentarily.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

* 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`
@kfarnung
Copy link
Contributor Author

Rebased one last time and new CI run: https://travis-ci.org/kfarnung/node-addon-api/builds/253725282

@mhdawson
Copy link
Member

Looks like Ben's comments have been addressed so landing.

mhdawson pushed a commit that referenced this pull request Jul 14, 2017
* 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 <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

landed as 28fad43

@mhdawson mhdawson closed this Jul 14, 2017
@kfarnung kfarnung deleted the node_internal branch July 14, 2017 19:02
addaleax pushed a commit to nodejs/node that referenced this pull request Jul 18, 2017
V8 caches and does not subsequently release `ObjectTemplate` instances.
Thus, we need to store the `ObjectTemplate` from which we derive object
instances we use for `napi_wrap()` and function/accessor context in a
persistent in the `napi_env`.

nodejs/node-addon-api#70 (comment)

PR-URL: #13999
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
Fishrock123 pushed a commit to nodejs/node that referenced this pull request Jul 19, 2017
V8 caches and does not subsequently release `ObjectTemplate` instances.
Thus, we need to store the `ObjectTemplate` from which we derive object
instances we use for `napi_wrap()` and function/accessor context in a
persistent in the `napi_env`.

nodejs/node-addon-api#70 (comment)

PR-URL: #13999
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
V8 caches and does not subsequently release `ObjectTemplate` instances.
Thus, we need to store the `ObjectTemplate` from which we derive object
instances we use for `napi_wrap()` and function/accessor context in a
persistent in the `napi_env`.

nodejs/node-addon-api#70 (comment)

PR-URL: nodejs#13999
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Apr 16, 2018
V8 caches and does not subsequently release `ObjectTemplate` instances.
Thus, we need to store the `ObjectTemplate` from which we derive object
instances we use for `napi_wrap()` and function/accessor context in a
persistent in the `napi_env`.

nodejs/node-addon-api#70 (comment)

Backport-PR-URL: #19447
PR-URL: #13999
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
* 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: nodejs/node-addon-api#70
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
* 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: nodejs/node-addon-api#70
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
* 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: nodejs/node-addon-api#70
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
* 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: nodejs/node-addon-api#70
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Michael Dawson <[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 this pull request may close these issues.

5 participants