lib: add internal check macros #18852

devsnek · 2018-02-18T16:58:12Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

devsnek · 2018-02-18T18:04:09Z

lib/internal/bootstrap_node.js

+  const check = (expr) => {
+    if (expr)
+      return;
+    const e = new Error(`${expr} == true`);


this will just be a boolean for all the CHECK_* macros that use this, and won't be very descriptive, which might be kinda annoying.

bnoordhuis · 2018-02-18T18:59:25Z

configure

+  if options.lib_debug:
+    o['variables']['node_lib_debug'] = 1
+  else:
+    o['variables']['node_lib_debug'] = 'false'


o['variables']['node_lib_debug'] = b(options.lib_debug)

(I get that you copied it from the http2 flags but that's a really unidiomatic way of doing it.)

bnoordhuis · 2018-02-18T19:08:30Z

node.gyp

@@ -740,6 +740,9 @@
            }],
            [ 'node_use_perfctr=="false"', {
              'inputs': [ 'src/noperfctr_macros.py' ]
+            }],
+            [ 'node_lib_debug=="false"', {
+              'inputs': [ 'src/check_macros.py' ]


Better name: nocheck_macro.py

That said, if you're using macros anyway, have two files: one with no-ops and one with, uh, ops, I guess?

macro CHECK(x) = do { if (!(x)) throw new Error(`x`); } while (0);

For the no-ops, instead of evaluating to nothing, maybe emit something that is dead code but syntax-checked, to avoid bitrot:

macro CHECK(x) = while (x, 0) {};

And a third suggestion is to have a guard variable so you can enable/disable checks at run-time instead of compile time.

devsnek · 2018-02-18T19:35:04Z

it looks like js2c has a bug with expanding the macros (CHECK_EQ(true, false) becomes void(true, b))

i honestly have no idea what is going on in js2c so if someone could help out that would be nice 😄

BridgeAR

I think it would be great to add a single "test" entry. That could of course also be in a separate commit.

BridgeAR · 2018-02-21T16:47:37Z

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

devsnek · 2018-02-21T16:59:52Z

@BridgeAR this isn't ready for ci or merge, it doesn't actually work yet, see #18852 (comment)

BridgeAR · 2018-02-21T17:02:36Z

@devsnek I was not sure if the comment would still apply because there was a code change after your comment. And it has often been unnoticed if something was addressed. So I just thought I start a CI, no matter if it still applies or not :-)

Can someone help out? Ping @jasnell @addaleax @bnoordhuis

joyeecheung · 2018-02-22T11:40:32Z

Is there a way to combine this with NODE_DEBUG env used in util.debuglog? Maybe we can make CHECK a noop when there isn't a NODE_DEBUG environment variable set? Like CHECK = internalUtil.debugCheck('prefix'). Not sure how much the overhead is compared to the js2c approach.

joyeecheung · 2018-02-22T11:45:52Z

Also it would be great if we have both CHECK (activated in normal build) and DCHECK (activated in debug build..or with NODE_DEBUG?), that way we can replace some asserts in the code with CHECK and create a special error code for those (right now they are just ERR_ASSERTION which is also user-facing)

I think we can even turn on NODE_DEBUG=* on CIs to improve the debuggability of flakes. If the macro has to be activated through a debug build then the CI would not benefit much from it.

BridgeAR · 2018-02-22T11:57:10Z

I personally also want to replace all assert calls with this macro. I am definitely found of getting this in.

I do not think that we need a CHECK. I think we should only need a DCHECK. Because a CHECK would actually better be replaced with a if (foo) throw ...

joyeecheung · 2018-02-22T12:08:12Z

@BridgeAR If by throwing an error you are talking about creating public errors, I took a look at the assert in the lib and I don't think most of them should be turned into if (foo) throw .., because when the assertion is false, that basically means the user is messing up with the internals with monkey-patching or private APIs, or there is a bug in our code.

mmarchini · 2018-02-22T14:48:59Z

@devsnek I added an in progress label, feel free to remove once it's ready to CI/land 😄

BridgeAR · 2018-02-23T10:48:03Z

@joyeecheung I agree that the current assert calls in /lib should not be turned into if (foo) throw .... So far most of those feel like being ideal to be replaced with a DCHECK because I think they are mainly there for debugging while working on the code.
I am not certain if the assert calls make sense to guard against messed up monkey-patching because that is something that is questionable on it's own and if someone wants to do that, I feel they should make sure everything works on their own. Don't you think so?

joyeecheung · 2018-02-23T14:01:33Z

@BridgeAR

I am not certain if the assert calls make sense to guard against messed up monkey-patching because that is something that is questionable on it's own and if someone wants to do that, I feel they should make sure everything works on their own. Don't you think so?

I believe monkey-patching is still inevitable in many cases (e.g. APM modules, but the async hooks and the upcoming loader hooks could help them with that), and these asserts is a way to help them make sure everything works on their own.

BridgeAR · 2018-03-02T01:41:06Z

@devsnek it seems like we use a very old version of js2c.py. Without any further knowledge about it: you could see if everything works if you update it to the v8 deps one.

devsnek · 2018-03-02T03:05:40Z

@BridgeAR thanks for the tip, this is working now 🎉

bnoordhuis · 2018-03-02T16:19:30Z

Potential future follow-up: make stringification work so that #x in process._rawDebug(#x) expands to the failed check as a string.

BridgeAR · 2018-03-02T16:22:48Z

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

devsnek · 2018-03-02T16:35:34Z

@BridgeAR @bnoordhuis needs another lookover with the new commits

BridgeAR · 2018-03-02T16:38:39Z

I personally would rather migrate those in a different PR

devsnek · 2018-03-02T17:44:27Z

@BridgeAR i removed them

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

devsnek · 2018-03-02T22:25:40Z

failures appear unrelated:

windows: https://ci.nodejs.org/job/node-test-binary-windows/COMPILED_BY=vs2017-x86,RUNNER=win2012r2,RUN_SUBSET=0/15411/console

ubuntu1604_sharedlibs_debug_x64

14:21:44 not ok 715 parallel/test-http-dump-req-when-res-ends
14:21:44   ---
14:21:44   duration_ms: 9.527
14:21:44   severity: fail
14:21:44   stack: |-
14:21:44     resume called
14:21:44     Mismatched  function calls. Expected at least 1, actual 0.
14:21:44         at Object.exports.mustCallAtLeast (/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_debug_x64/test/common/index.js:431:10)
14:21:44         at IncomingMessage. (/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_debug_x64/test/parallel/test-http-dump-req-when-res-ends.js:12:27)
14:21:44         at IncomingMessage. (/home/iojs/build/workspace/node-test-commit-linux-containered/nodes/ubuntu1604_sharedlibs_debug_x64/test/common/index.js:467:15)
14:21:44         at IncomingMessage.emit (events.js:131:13)
14:21:44         at resume_ (_stream_readable.js:833:10)
14:21:44         at process._tickCallback (internal/process/next_tick.js:115:19)

devsnek · 2018-03-05T14:36:14Z

landed in 3ed363c

PR-URL: #18852 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins · 2018-03-06T21:41:56Z

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

joyeecheung · 2018-03-06T22:54:20Z

Just noticed that CHECK abort instead of throwing, which means we probably cannot replace most assert in lib with this

BridgeAR · 2018-03-06T22:58:43Z

@joyeecheung we can revise that. I personally do not really have a strong opinion either way. I would just like the assert calls be replaced. If we have to change the abort for that again, I am all for it.

PR-URL: #18852 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

PR-URL: nodejs#18852 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

jasnell · 2018-08-17T22:43:26Z

Should this be backport to 8.x? If so, a separate backport PR is necessary.

nodejs-github-bot added build Issues and PRs related to build files or the CI. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 18, 2018

devsnek force-pushed the feature/internal-check-macro branch 2 times, most recently from 75bd7b6 to 3087898 Compare February 18, 2018 17:54

devsnek commented Feb 18, 2018

View reviewed changes

devsnek force-pushed the feature/internal-check-macro branch from 3087898 to e2612ed Compare February 18, 2018 18:35

devsnek changed the title ~~tools, lib: add internal check macros~~ lib: add internal check macros Feb 18, 2018

bnoordhuis reviewed Feb 18, 2018

View reviewed changes

devsnek force-pushed the feature/internal-check-macro branch from e2612ed to 9dc1e9e Compare February 18, 2018 19:24

devsnek force-pushed the feature/internal-check-macro branch from 9dc1e9e to 4495cf6 Compare February 18, 2018 19:47

BridgeAR reviewed Feb 21, 2018

View reviewed changes

mmarchini added the wip Issues and PRs that are still a work in progress. label Feb 22, 2018

devsnek force-pushed the feature/internal-check-macro branch from 4495cf6 to 5555b67 Compare March 2, 2018 03:05

devsnek force-pushed the feature/internal-check-macro branch from 5555b67 to 15e5618 Compare March 2, 2018 03:07

bnoordhuis approved these changes Mar 2, 2018

View reviewed changes

BridgeAR removed the wip Issues and PRs that are still a work in progress. label Mar 2, 2018

devsnek force-pushed the feature/internal-check-macro branch from 15e5618 to a6d6e56 Compare March 2, 2018 16:32

devsnek removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2018

devsnek force-pushed the feature/internal-check-macro branch from a6d6e56 to 16af089 Compare March 2, 2018 16:38

devsnek force-pushed the feature/internal-check-macro branch from 16af089 to 3ab6635 Compare March 2, 2018 17:42

devsnek added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2018

devsnek force-pushed the feature/internal-check-macro branch from 3ab6635 to 9165e85 Compare March 2, 2018 18:21

lib: add internal check macros

5dc47a4

devsnek force-pushed the feature/internal-check-macro branch from 9165e85 to 5dc47a4 Compare March 2, 2018 18:29

devsnek closed this Mar 5, 2018

devsnek deleted the feature/internal-check-macro branch March 5, 2018 14:36

devsnek added a commit that referenced this pull request Mar 5, 2018

lib: add internal check macros

3ed363c

PR-URL: #18852 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

MylesBorins added the backport-requested-v9.x label Mar 6, 2018

devsnek removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 18, 2018

richardlau mentioned this pull request Mar 22, 2018

JavaScript debug statements #18851

Closed

targos removed the backport-requested-v9.x label Apr 2, 2018

targos pushed a commit that referenced this pull request Apr 2, 2018

lib: add internal check macros

d3d1ee7

PR-URL: #18852 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

targos mentioned this pull request Apr 4, 2018

v9.11.0 proposal #19799

Merged

MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018

lib: add internal check macros

d4c340e

PR-URL: nodejs#18852 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

jasnell added the backport-requested-v8.x label Aug 17, 2018

targos mentioned this pull request Nov 18, 2018

http2: replace unreachable error with assertion #24407

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: add internal check macros #18852

lib: add internal check macros #18852

devsnek commented Feb 18, 2018 •

edited

Loading

devsnek Feb 18, 2018

bnoordhuis Feb 18, 2018

bnoordhuis Feb 18, 2018

devsnek commented Feb 18, 2018

BridgeAR left a comment

BridgeAR commented Feb 21, 2018

devsnek commented Feb 21, 2018

BridgeAR commented Feb 21, 2018

joyeecheung commented Feb 22, 2018

joyeecheung commented Feb 22, 2018 •

edited

Loading

BridgeAR commented Feb 22, 2018

joyeecheung commented Feb 22, 2018 •

edited

Loading

mmarchini commented Feb 22, 2018

BridgeAR commented Feb 23, 2018

joyeecheung commented Feb 23, 2018 •

edited

Loading

BridgeAR commented Mar 2, 2018

devsnek commented Mar 2, 2018

bnoordhuis commented Mar 2, 2018

BridgeAR commented Mar 2, 2018

devsnek commented Mar 2, 2018

BridgeAR commented Mar 2, 2018

devsnek commented Mar 2, 2018 •

edited

Loading

devsnek commented Mar 2, 2018 •

edited

Loading

devsnek commented Mar 5, 2018

MylesBorins commented Mar 6, 2018

joyeecheung commented Mar 6, 2018

BridgeAR commented Mar 6, 2018

jasnell commented Aug 17, 2018

lib: add internal check macros #18852

lib: add internal check macros #18852

Conversation

devsnek commented Feb 18, 2018 • edited Loading

Checklist

Affected core subsystem(s)

devsnek Feb 18, 2018

Choose a reason for hiding this comment

bnoordhuis Feb 18, 2018

Choose a reason for hiding this comment

bnoordhuis Feb 18, 2018

Choose a reason for hiding this comment

devsnek commented Feb 18, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR commented Feb 21, 2018

devsnek commented Feb 21, 2018

BridgeAR commented Feb 21, 2018

joyeecheung commented Feb 22, 2018

joyeecheung commented Feb 22, 2018 • edited Loading

BridgeAR commented Feb 22, 2018

joyeecheung commented Feb 22, 2018 • edited Loading

mmarchini commented Feb 22, 2018

BridgeAR commented Feb 23, 2018

joyeecheung commented Feb 23, 2018 • edited Loading

BridgeAR commented Mar 2, 2018

devsnek commented Mar 2, 2018

bnoordhuis commented Mar 2, 2018

BridgeAR commented Mar 2, 2018

devsnek commented Mar 2, 2018

BridgeAR commented Mar 2, 2018

devsnek commented Mar 2, 2018 • edited Loading

devsnek commented Mar 2, 2018 • edited Loading

devsnek commented Mar 5, 2018

MylesBorins commented Mar 6, 2018

joyeecheung commented Mar 6, 2018

BridgeAR commented Mar 6, 2018

jasnell commented Aug 17, 2018

devsnek commented Feb 18, 2018 •

edited

Loading

joyeecheung commented Feb 22, 2018 •

edited

Loading

joyeecheung commented Feb 22, 2018 •

edited

Loading

joyeecheung commented Feb 23, 2018 •

edited

Loading

devsnek commented Mar 2, 2018 •

edited

Loading

devsnek commented Mar 2, 2018 •

edited

Loading