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 napi_fatal_error API #13971

Closed
wants to merge 1 commit into from
Closed

Conversation

kfarnung
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

n-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Jun 28, 2017
@kfarnung
Copy link
Contributor Author

/cc @jasongin @mhdawson

doc/api/n-api.md Outdated
- `[in] location`: The location at which the error occurred.
- `[in] message`: The message associated with the error.

Call does not return, the process will be terminated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Call -> The function call

@@ -772,6 +772,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);
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this require node internals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, but since it was already included in the file I assumed it wasn't a big deal. Since this is the v8-specific implementation of N-API, this can still be considered internal (I think).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I forgot this hadn't landed. #13892

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, ok, I hadn't seen that. It seems like it would be best then to adapt an implementation into node_api.cc that mimics the node::FatalError behavior?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that is what I had expected we would need to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on discussion today in the WG meeting, we'll be reverting back to the original change and removing the duplicated code.

"targets": [
{
"target_name": "test_fatal",
"sources": [ "test_fatal.cc" ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason not to use just C?

#include <node_api.h>
#include "../common.h"

#if defined _WIN32
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need windows.h and/or unistd.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, I meant to try removing this and forgot.

@kfarnung kfarnung force-pushed the fatal_error branch 2 times, most recently from 434945d to 70e0919 Compare June 28, 2017 22:14
src/node_api.cc Outdated
fflush(stderr);
ABORT();
// to suppress compiler warning
ABORT();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, I'll clean this up.

@kfarnung kfarnung force-pushed the fatal_error branch 2 times, most recently from fc27c86 to 44bcf8e Compare June 29, 2017 00:05
@kfarnung
Copy link
Contributor Author

@cjihrig @jasongin I grabbed the relevant portions of the node::FatalError method directly into node_api.cc, but it feels a little dirty to have to copy the PrintErrorString method. I don't see any other instances of print in node_api.cc, so I wasn't sure how else to do it.

@kfarnung
Copy link
Contributor Author

Resolves #13927

const test_fatal = require(`./build/${common.buildType}/test_fatal`);

// Exception thrown from async completion callback.
// (Tested in a spawned process because the exception is fatal.)
Copy link
Member

Choose a reason for hiding this comment

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

I recognize these comments, they were copy-pasted from an async callback test I wrote. Please update them. :)

src/node_api.cc Outdated

// Don't include the null character in the output
CHECK_GT(n, 0);
WriteConsoleW(stderr_handle, wbuf.data(), n - 1, nullptr, nullptr);
Copy link
Member

Choose a reason for hiding this comment

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

Does anyone know the reason for this special case of calling WriteConsoleW() for a windows console? node::Assert() is very similar to node::FatalError(), but it just does a regular fprintf(stderr, ...).

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I think it's required for proper display of UTF-8 encoded error messages? I guess assertion messages are more likely to be limited to plain ASCII. I suppose we do want UTF-8 capability here.

doc/api/n-api.md Outdated
```

- `[in] location`: The location at which the error occurred.
- `[in] message`: The message associated with the error.
Copy link
Member

Choose a reason for hiding this comment

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

Make it clear that location is NULL-able, while message isn't. Speaking of that, I'd actually prefer the function signature to be (message, location) as it seems customary for NULL-able parameters to go last.

Copy link
Member

Choose a reason for hiding this comment

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

Speaking of this, maybe it would be helpful if message can be a printf format string instead? That'd invalidate my comment above but would provide callers more flexibility.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would lead to mistakes when people are migrating code to N-API if the parameter order is different from node::FatalError().

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Copy/pasting large chunks of code from src/node.cc is not acceptable. See also #13892 (comment).

@mhdawson
Copy link
Member

Based on discussion today in N-API meeting we are going to look at having a version of the node internals file as part of the wrapper so that we can avoid having to have copies in the core Node.js codebase.

@kfarnung
Copy link
Contributor Author

kfarnung commented Jul 5, 2017

@bnoordhuis Can you take another look? I've eliminated all of the code duplication in node source files.

CI: https://ci.nodejs.org/job/node-test-pull-request/8996

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM with a style nit.

}

const p = child_process.spawnSync(
process.execPath, [ '--napi-modules', __filename, 'child' ]);
Copy link
Member

Choose a reason for hiding this comment

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

4 space indent here and below (line continuation.)

};

NAPI_CALL_RETURN_VOID(env, napi_define_properties(
env, exports, sizeof(properties) / sizeof(*properties), properties));
Copy link
Member

Choose a reason for hiding this comment

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

Tangential: macros like NAPI_ASSERT_BASE and NAPI_CALL_BASE should have their bodies wrapped in do { } while (0) in order for them to be safe to use in if/else statements. Right now they can create bugs that will be hard to figure out. Something for another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opened PR #14095 to fix this.

@kfarnung kfarnung force-pushed the fatal_error branch 2 times, most recently from 4068780 to 6a32422 Compare July 6, 2017 22:53
@kfarnung
Copy link
Contributor Author

kfarnung commented Jul 7, 2017

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

@kfarnung
Copy link
Contributor Author

kfarnung commented Jul 7, 2017

@mhdawson @jasongin I believe this is ready to land whenever someone is ready to do it.

@kfarnung
Copy link
Contributor Author

Rebased and new CI: https://ci.nodejs.org/job/node-test-pull-request/9113/

@mhdawson
Copy link
Member

Failed in CI on PPCBE is known issue.

@mhdawson
Copy link
Member

Failure on Alpine is known issue.

@mhdawson
Copy link
Member

Linux fips issue was a machine issue.

@mhdawson
Copy link
Member

Ok CI completed, landing.

mhdawson pushed a commit that referenced this pull request Jul 13, 2017
PR-URL: #13971
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
Copy link
Member

landed as 73078d6

@mhdawson mhdawson closed this Jul 13, 2017
@kfarnung kfarnung deleted the fatal_error branch July 13, 2017 23:10
@addaleax addaleax mentioned this pull request Jul 18, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
PR-URL: #13971
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
PR-URL: #13971
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 10, 2018
PR-URL: nodejs#13971
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
Backport-PR-URL: #19447
PR-URL: #13971
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jason Ginchereau <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants