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

doc: suggest not to throw JS errors from C++ #18149

Closed
wants to merge 3 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 59 additions & 6 deletions CPP_STYLE_GUIDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@
* [Others](#others)
* [Type casting](#type-casting)
* [Do not include `*.h` if `*-inl.h` has already been included](#do-not-include-h-if--inlh-has-already-been-included)
* [Avoid throwing JavaScript errors in nested C++ methods](#avoid-throwing-javascript-errors-in-nested-c-methods)
* [Avoid throwing JavaScript errors in C++ methods](#avoid-throwing-javascript-errors-in-c)
* [Avoid throwing JavaScript errors in nested C++ methods](#avoid-throwing-javascript-errors-in-nested-c-methods)

Unfortunately, the C++ linter (based on
[Google’s `cpplint`](https://github.com/google/styleguide)), which can be run
Expand Down Expand Up @@ -213,12 +214,64 @@ instead of
#include "util-inl.h"
```

## Avoid throwing JavaScript errors in nested C++ methods
## Avoid throwing JavaScript errors in C++

If you need to throw JavaScript errors from a C++ binding method, try to do it
at the top level and not inside of nested calls.
When there is a need to throw errors from a C++ binding method, try to
return the data necessary for constructing the errors to JavaScript,
then construct and throw the errors [using `lib/internal/errors.js`][errors].

A lot of code inside Node.js is written so that typechecking etc. is performed
in JavaScript.
Note that in general, type-checks on arguments should done in JavaScript
Copy link
Member

Choose a reason for hiding this comment

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

should be done

Copy link
Member Author

Choose a reason for hiding this comment

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

@targos Fixed, thanks for catching that!

before the arguments are passed into C++.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add something about including a CHECK at the C++ level?


If the return value of the binding cannot be used to signal failures or return
the necessary data for constructing errors in JavaScript, pass a context object
to the binding and put the necessary data inside in C++. For example:

```c++
void Foo(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
// Let the JavaScript handle the actual type-checking,
// only assertions are placed in C++
CHECK_EQ(args.Length(), 2);
CHECK(args[0]->IsString());
CHECK(args[1]->IsObject());

int err = DoSomethingWith(args[0].As<String>());
if (err) {
// Put the data inside the error context
Local<Object> ctx = args[1].As<Object>();
Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "code")
ctx->Set(env->context(), key, err);
} else {
args.GetReturnValue().Set(something_to_return);
}
}

// In the initialize function
env->SetMethod(target, "foo", Foo);
```

```js
exports.foo = function(str) {
// Prefer doing the type-checks in JavaScript
if (typeof str !== 'string') {
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'str', 'string');
}

const ctx = {};
const result = binding.foo(str, ctx);
if (ctx.code !== undefined) {
throw errors.Error('ERR_ERROR_NAME', ctx.code);
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing new?

}
return result;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

A nit: missing semicolon.

```

### Avoid throwing JavaScript errors in nested C++ methods

When you have to throw the errors from C++, try to do it at the top level and
not inside of nested calls.
Copy link
Member

Choose a reason for hiding this comment

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

"Maybe" mention Maybe<> and MaybeLocal<> as possible methods to pass exception status if exceptions have to be transported through nested C++ calls.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm I think we could make a separate section for the Maybes and link from here?

Copy link
Member

Choose a reason for hiding this comment

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

Either is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

@TimothyGu Since this was how it's documented prior to this PR, I'll open a separate PR for the usage of Maybes. We also need a section about using the Maybe APIs instead of the deprecated V8 APIs.


Using C++ `throw` is not allowed.

[errors]: https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md