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

[Tracking] Converting C++ errors to use internal/errors #18106

Closed
jasnell opened this issue Jan 11, 2018 · 3 comments
Closed

[Tracking] Converting C++ errors to use internal/errors #18106

jasnell opened this issue Jan 11, 2018 · 3 comments
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core.

Comments

@jasnell
Copy link
Member

jasnell commented Jan 11, 2018

This is a tracking issue for the remaining bits of work around converting errors thrown in C++ land over to use internal/errors. There is still quite a bit here that needs to be done.

Currently, quite a few functions defined at the C++ layer and exposed through the process.bindings() perform input argument type checking at the C++ level and use some variation of env->ThrowTypeError() to report the error back up to JavaScript. As much as possible, these should be modified to move the input argument type checking up into the JavaScript layer before the binding function is called, and the checks occurring at the C++ level should be changed into in appropriate CHECK() statement. There are quite a few of these and it will be difficult to enumerate them all here. (Note also that this should include range checking)

The general pattern for most functions is: Type check the inputs, perform some function, handle the result. In many cases, if the result is an error, some variation of env->ThrowError() is used to report the error back up to Javascript. As much as possible, these should be converted to return an error code back to JavaScript in some way then have the actual Error object generated within JavaScript.

There are a few very rare cases where the C++ objects and functions are exposed directly to end users as public APIs. In these cases, we either need to introduce a JavaScript wrapper or we will have to add new internal C++ env->Throw variants that accept an error code.

Overall, the goal is to move as many of the Error creations into JavaScript as possible, then deal with the ones we absolutely cannot move separately.

/cc @mhdawson

@joyeecheung
Copy link
Member

joyeecheung commented Jan 11, 2018

To avoid stepping on anyone's toes: I am working on those errors in fs (#17914 is where I am currently). Also have a branch locally to migrate the synchronous errors in tls (still trying to come up with better tests..).

@joyeecheung joyeecheung added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Jan 11, 2018
@mhdawson
Copy link
Member

@komawar a good way to start helping out on this front would be to

  • read/digest the above
  • figure out the first one to move (believe we should do them 1 at a time or in small groups to make reviewing easlier)
  • I can help out if needed just ping me once you have taken a look and we can get together to discuss.

joyeecheung added a commit that referenced this issue Jan 16, 2018
Also provide an example on how to use internal/errors
to handle errors in C++.

PR-URL: #18149
Refs: #18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
evanlucas pushed a commit that referenced this issue Jan 30, 2018
Also provide an example on how to use internal/errors
to handle errors in C++.

PR-URL: #18149
Refs: #18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
joyeecheung added a commit that referenced this issue Feb 1, 2018
PR-URL: #18348
Refs: #18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit that referenced this issue Feb 1, 2018
PR-URL: #18348
Refs: #18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit that referenced this issue Feb 1, 2018
PR-URL: #18348
Refs: #18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit that referenced this issue Feb 1, 2018
PR-URL: #18348
Refs: #18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit that referenced this issue Feb 1, 2018
PR-URL: #18348
Refs: #18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit that referenced this issue Feb 1, 2018
PR-URL: #18348
Refs: #18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit that referenced this issue Feb 1, 2018
PR-URL: #18348
Refs: #18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit that referenced this issue Feb 1, 2018
PR-URL: #18348
Refs: #18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
joyeecheung added a commit that referenced this issue Feb 27, 2018
PR-URL: #18871
Refs: #18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
joyeecheung added a commit that referenced this issue Feb 27, 2018
PR-URL: #18871
Refs: #18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
joyeecheung added a commit that referenced this issue Feb 27, 2018
PR-URL: #18871
Refs: #18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
joyeecheung added a commit that referenced this issue Feb 27, 2018
PR-URL: #18871
Refs: #18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
joyeecheung added a commit that referenced this issue Feb 27, 2018
PR-URL: #18871
Refs: #18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
joyeecheung added a commit that referenced this issue Feb 27, 2018
PR-URL: #18871
Refs: #18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
targos pushed a commit that referenced this issue Mar 22, 2018
PR-URL: #19268
Refs: #18106
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18348
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18348
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18348
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18348
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18348
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18348
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18348
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18348
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18871
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18871
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18871
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18871
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18871
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18871
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18871
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18871
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18871
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18871
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
PR-URL: nodejs#18871
Refs: nodejs#18106
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@Trott
Copy link
Member

Trott commented Jul 5, 2019

This isn't really a tracking issue, since there's no tracking happening. While it's true that we'd like to migrate as many C++ errors as possible/reasonable to use .code etc., I'm not sure this issue is doing it's job and I'm trying to get the issue tracker to be a bit less overwhelmed with stuff that isn't really being used. So I'm going to close this. Of course, if you think this is a bad idea and this issue should remain open, please re-open. (I'd ask that we at least try to maintain a list of files that need errors converted or something quantifiable. But that's just a request.)

@Trott Trott closed this as completed Jul 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core.
Projects
None yet
Development

No branches or pull requests

4 participants