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

Catch all exceptions #601

Closed
dmitryash opened this issue Nov 16, 2019 · 4 comments
Closed

Catch all exceptions #601

dmitryash opened this issue Nov 16, 2019 · 4 comments
Labels

Comments

@dmitryash
Copy link
Contributor

Currently only Error is caught. It is better to catch all exceptions otherwise an user has to use try..catch in each callback/function.

@gabrielschulhof
Copy link
Contributor

We catch only exceptions with which we know what to do. We know that we can turn a thrown Napi::Error into a JavaScript exception. We wouldn't know what to do with a std::exception, for example. The only thing we could do is

Napi::Error::New(env, "CallbackWrapper", "A native exception was thrown")
  .ThrowAsJavaScriptException();

before returning to JavaScript. This would mean that we basically swallow the native exception and replace it with a Napi::Error. This, in turn, would mean that the original exception is lost.

IMO if applications throw something other than Napi::Error, then we shouldn't handle it. Let the application crash. If the addon author wants to handle non-Napi::Error exceptions, we should give them the opportunity, because they might know how to intelligently turn a non-Napi::Error into a Napi::Error.

We could start adding convenience handlers for, like, int, and std::string, and const char* and so on, but converting those to Napi::Error should really not be up to us.

@dmitryash
Copy link
Contributor Author

IMO if applications throw something other than Napi::Error, then we shouldn't handle it. Let the application crash.

Letting a whole nodejs crash is really a worse idea.

We could start adding convenience handlers for, like, int, and std::string, and const char* and so on, but converting those to Napi::Error should really not be up to us.

A customizable exception mapper is nice feature. However the default one can catch only Error, std::exception and ...:

try {
    return callback();
} catch (const Error& e) {
    e.ThrowAsJavaScriptException();
} catch (const std::exception &e) {
    Napi::Error::New(env, "CallbackWrapper", e.what()).ThrowAsJavaScriptException();
} catch (...) {
    Napi::Error::New(env, "CallbackWrapper", "A native exception was thrown").ThrowAsJavaScriptException();
}
return nullptr;

Throw anything except derived from std::exception is a bad practice. However an user can define his own exception mapper.

@seishun
Copy link
Contributor

seishun commented Dec 4, 2020

We wouldn't know what to do with a std::exception, for example.

Why not call std::exception::what on it? I assumed that's what happens when I read this sentence in the documentation:

On return from a native method, node-addon-api will automatically convert a pending C++ exception to a JavaScript exception.

This would have saved us a lot of boilerplate.

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2021

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants