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.

Sign up for GitHub

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

Jump to bottom

Can't use node::FatalError() #232

Closed
gabrielschulhof opened this issue Apr 17, 2017 · 12 comments
Closed

Can't use node::FatalError() #232

gabrielschulhof opened this issue Apr 17, 2017 · 12 comments

Comments

@gabrielschulhof
Copy link
Collaborator

... because node_internals.h is not available outside of node. So, if we want node-api to contain unaltered versions of the implementation, we must not rely on any node internals.

I guess we could simply throw a JS exception. Since Isolate::GetCurrent() is going away, and since we don't have an env from which to extract an isolate, is there some other way we can signal that we wish to throw an exception?
@bnoordhuis
Copy link
Member

I guess we could simply throw a JS exception.

FatalError() is a noreturn function. Its n-api equivalent shouldn't throw an exception, it should exit.

@addaleax
Copy link
Member

Just copying Node’s FatalError, i.e. fprintf() + fflush() + abort(), sounds good to me too.

Since Isolate::GetCurrent() is going away

Just curious, is that publicly announced anywhere?

@bnoordhuis
Copy link
Member

Just curious, is that publicly announced anywhere?

Not really, but V8 people have repeatedly indicated over the years they want to get rid of it.

@gabrielschulhof
Copy link
Collaborator Author

@addaleax well, it's actually more than that, because in Windows it's raise(SIGABRT), and not fprintf() but WriteConsoleW(), etc. So, there's quite a bit of code in node.cc dealing with this.

@gabrielschulhof
Copy link
Collaborator Author

@bnoordhuis we were originally debating whether we should consider the absence of an env a fatal error or not. There was a good argument for "not" in that this is just a module, so perhaps the whole app shouldn't be shot down because of a buggy module. The reason we went with the "aye" in the end was that it's very much a corner case anyway.

@gabrielschulhof
Copy link
Collaborator Author

But without an env, and hence without an isolate, we can't even throw an exception.

@gabrielschulhof
Copy link
Collaborator Author

We can wrap the call to node::FatalError() in #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS to avoid failure to compile outside of node, but we should really have something in the #else case.

@jasongin
Copy link
Member

I'm still not convinced we need FatalError() functionality. The one place where it is used now could return a napi_status error instead.

@jasongin
Copy link
Member

Related: nodejs/node-addon-api#25

gabrielschulhof pushed a commit to gabrielschulhof/abi-stable-node that referenced this issue Apr 20, 2017
n-api: provide node::FatalError() when internals are absent
749d89e 
src/node_api.cc must build both as a part of node and as an addon
against an older version of node, so that it may be possible to make
N-API available on older, already-published versions of node. Thus,
it must not make use of node internals such as node::FatalError().

Yet N-API must also exit with a fatal error in the absence of a
napi_env pointer. To that end, node::FatalError() is implemented if
internals are unavailable by copying PrintErrorString() from node.cc,
simplifying it to accept only a location and an error message, and
augmenting it with fflush() and platform-ifdefed abort(), the latter
two of which are also copied from node.cc.

Re nodejs#232
Re https://github.com/nodejs/node-api
@jasongin jasongin mentioned this issue May 3, 2017
Merged
@jasongin
Copy link
Member

jasongin commented May 4, 2017

We might want to actually expose node::FatalError() (or similar) as a napi_fatal_error() API. See nodejs/node-addon-api#32 (comment) for a scenario where it is needed.

@jasongin jasongin mentioned this issue May 4, 2017
Merged
This was referenced May 30, 2017
Closed
Closed
@mhdawson
Copy link
Member

nodejs/node#13927

@digitalinfinity
Copy link
Contributor

Addressed by nodejs/node#13971

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@digitalinfinity @bnoordhuis @addaleax @gabrielschulhof @mhdawson @jasongin