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

lib: add internal check macros #18852

Closed
wants to merge 1 commit into from

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Feb 18, 2018

Refs #18851

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

@nodejs-github-bot 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 devsnek force-pushed the feature/internal-check-macro branch 2 times, most recently from 75bd7b6 to 3087898 Compare February 18, 2018 17:54
const check = (expr) => {
if (expr)
return;
const e = new Error(`${expr} == true`);
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@devsnek devsnek changed the title tools, lib: add internal check macros lib: add internal check macros Feb 18, 2018
configure Outdated
if options.lib_debug:
o['variables']['node_lib_debug'] = 1
else:
o['variables']['node_lib_debug'] = 'false'
Copy link
Member

Choose a reason for hiding this comment

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

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.)

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

Choose a reason for hiding this comment

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

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
Copy link
Member Author

devsnek commented Feb 18, 2018

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 😄

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

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

@BridgeAR
Copy link
Member

@devsnek
Copy link
Member Author

devsnek commented Feb 21, 2018

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

@BridgeAR
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

joyeecheung commented Feb 22, 2018

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
Copy link
Member

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
Copy link
Member

joyeecheung commented Feb 22, 2018

@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 mmarchini added the wip Issues and PRs that are still a work in progress. label Feb 22, 2018
@mmarchini
Copy link
Contributor

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

@BridgeAR
Copy link
Member

@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
Copy link
Member

joyeecheung commented Feb 23, 2018

@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
Copy link
Member

BridgeAR commented Mar 2, 2018

@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 devsnek force-pushed the feature/internal-check-macro branch from 4495cf6 to 5555b67 Compare March 2, 2018 03:05
@devsnek
Copy link
Member Author

devsnek commented Mar 2, 2018

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

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

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

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

BridgeAR commented Mar 2, 2018

@devsnek devsnek force-pushed the feature/internal-check-macro branch from 15e5618 to a6d6e56 Compare March 2, 2018 16:32
@devsnek 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
Copy link
Member Author

devsnek commented Mar 2, 2018

@BridgeAR @bnoordhuis needs another lookover with the new commits

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

BridgeAR commented Mar 2, 2018

I personally would rather migrate those in a different PR

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

devsnek commented Mar 2, 2018

@devsnek 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 devsnek force-pushed the feature/internal-check-macro branch from 3ab6635 to 9165e85 Compare March 2, 2018 18:21
@devsnek devsnek force-pushed the feature/internal-check-macro branch from 9165e85 to 5dc47a4 Compare March 2, 2018 18:29
@devsnek
Copy link
Member Author

devsnek commented Mar 2, 2018

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
Copy link
Member Author

devsnek commented Mar 5, 2018

landed in 3ed363c

@devsnek devsnek closed this Mar 5, 2018
@devsnek 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
PR-URL: #18852
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
Copy link
Contributor

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
Copy link
Member

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

@BridgeAR
Copy link
Member

BridgeAR commented Mar 6, 2018

@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.

@devsnek 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
targos pushed a commit that referenced this pull request Apr 2, 2018
PR-URL: #18852
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@targos targos mentioned this pull request Apr 4, 2018
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
PR-URL: nodejs#18852
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@jasnell
Copy link
Member

jasnell commented Aug 17, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants