http2: replace unreachable error with assertion by Trott · Pull Request #24407 · nodejs/node

Trott · 2018-11-17T00:26:43Z

"That particular emit('error', ...) is largely defensively coded and
should not ever actually happen." Sounds like an assertion rather than
an error event. The code in question has no test coverage because it is
believed to be unreachable.

Fixes: #20673

Checklist

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

"That particular `emit('error', ...)` is largely defensively coded and should not ever actually happen." Sounds like an assertion rather than an error event. The code in question has no test coverage because it is believed to be unreachable. Fixes: nodejs#20673

nodejs-github-bot · 2018-11-17T00:26:45Z

@Trott build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1644/pipeline

Trott · 2018-11-17T00:27:39Z

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

Trott · 2018-11-17T00:27:48Z

@nodejs/http2

mcollina

LGTM

targos · 2018-11-17T12:15:09Z

How do we decide between using assert or a CHECK macro?

Trott · 2018-11-18T03:12:55Z

How do we decide between using assert or a CHECK macro?

Is there a CHECK macro available for us in .js files?

targos · 2018-11-18T11:45:54Z

Yes. CHECK macros were added in #18852

Trott · 2018-11-18T16:31:04Z

Yes. CHECK macros were added in #18852

Hmmm....in general, I prefer assert() so that we're not as dependent/tied to our build tools.

But I guess the real things to consider are:

Worth it to incur a tiny runtime cost for all users with assert()? Or better to avoid the cost with CHECK?
Worth it to have the check run for everyone everywhere with assert()? Or OK to have it run in limited situations with CHECK?

In this case, I think assert() is right because if there's a coding error in http2 that will result in this problem, it's likely to be found in the wild and not in a debug situation.

targos · 2018-11-18T16:36:49Z

I do not understand your reasoning. CHECK is not a debugging tool. It just uses another mechanism than assert() (with no runtime cost) to report the error. It would still trigger an error in the wild if there was a programming mistake in http2

Trott · 2018-11-19T02:14:28Z

I do not understand your reasoning.

I guess I do not understand how CHECK works and will have to look at that more closely....

Trott · 2018-11-20T18:54:06Z

It just uses another mechanism than assert() (with no runtime cost) to report the error. It would still trigger an error in the wild if there was a programming mistake in http2

My understanding is that the CHECK() macros (on the .js side of things) are not enabled in the executables that we build for release. Am I wrong?

refack · 2018-11-20T20:35:51Z

My understanding is that the CHECK() macros (on the .js side of things) are not enabled in the executables that we build for release. Am I wrong?

Ok so it's more subtle. CHECK*s are converted to plain old JS

node/tools/check_macros.py

Lines 1 to 7 in 5dc47a4

    
           macro CHECK(x) = do { if (!(x)) (process._rawDebug("CHECK: x == true"), process.abort()) } while (0); 
        
           macro CHECK_EQ(a, b) = CHECK((a) === (b)); 
        
           macro CHECK_GE(a, b) = CHECK((a) >= (b)); 
        
           macro CHECK_GT(a, b) = CHECK((a) > (b)); 
        
           macro CHECK_LE(a, b) = CHECK((a) <= (b)); 
        
           macro CHECK_LT(a, b) = CHECK((a) < (b)); 
        
           macro CHECK_NE(a, b) = CHECK((a) !== (b));

DCHECK*s are turned into an almost no-op.

node/tools/nodcheck_macros.py

Lines 1 to 7 in 5dc47a4

    
           macro DCHECK(x) = void(x); 
        
           macro DCHECK_EQ(a, b) = void(a, b); 
        
           macro DCHECK_GE(a, b) = void(a, b); 
        
           macro DCHECK_GT(a, b) = void(a, b); 
        
           macro DCHECK_LE(a, b) = void(a, b); 
        
           macro DCHECK_LT(a, b) = void(a, b); 
        
           macro DCHECK_NE(a, b) = void(a, b);

BTW: I think we should turn DCHECK*s into nothing since void(x) still evaluates x.

refack · 2018-11-20T20:42:55Z

I do not understand your reasoning. CHECK is not a debugging tool. It just uses another mechanism than assert() (with no runtime cost) to report the error.

CHECK is a strong sanity assertion a.k.a. "contract". It will abort the process, not even throw, and it does incur runtime cost.
DCHECK is the weaker debug only counterpart. It also has a tiny runtime cost in production, since it will still evaluates the arguments.

i.e. DCHECKs can help flush out bug in debug builds
CHECKs should be used only in a "Houston we have a problem" places we have to bail before we cause damage to the user.

Trott · 2018-11-20T21:14:22Z

So, in this case, it seems assert() is the right choice because it will display a JS stack trace etc. rather than a terrifying process abort thing. At least, that's my reading?

Trott · 2018-11-20T21:16:34Z

(What's the purpose of wrapping the code in do...while inside CHECK()?)

refack · 2018-11-20T22:20:26Z

(What's the purpose of wrapping the code in do...while inside CHECK()?)

It's an old C ~~trick~~ idiom to force the creation of a scope.

P.S. I think there are places where this is the definition of cruft.

targos · 2018-11-20T22:31:15Z

and it does incur runtime cost.

That's right, I should have said it has a small runtime cost compared to loading the assert module and calling it.

I obviously don't understand the purpose of CHECKs so please don't block this PR any longer because of me 😅. I thought that the macros were added with the goal to replace internal uses of assert (because if the error should be recoverable, then we should throw something with a code).

Trott · 2018-11-20T23:21:45Z

That's right, I should have said it has a small runtime cost compared to loading the assert module and calling it.

I've had it on my mental to-do list to make a tiny assert() (maybe as part of internal util?) for use that the bigger assert module can use under the hood. The idea would be for us to have a super-lightweight version to use in our internal code that doesn't have to load the entire assert module.

I won't be mad if someone else does that before I ever get to it. :-D

Trott · 2018-11-20T23:22:47Z

Landed in 566a694

"That particular `emit('error', ...)` is largely defensively coded and should not ever actually happen." Sounds like an assertion rather than an error event. The code in question has no test coverage because it is believed to be unreachable. Fixes: nodejs#20673 PR-URL: nodejs#24407 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

refack · 2018-11-21T14:58:20Z

I've had it on my mental to-do list to make a tiny assert() (maybe as part of internal util?) for use that the bigger assert module can use under the hood. The idea would be for us to have a super-lightweight version to use in our internal code that doesn't have to load the entire assert module.

I have a long lasting dream of getting assert to be promoted to a language feature, then be made into a V8 native.

"That particular `emit('error', ...)` is largely defensively coded and should not ever actually happen." Sounds like an assertion rather than an error event. The code in question has no test coverage because it is believed to be unreachable. Fixes: #20673 PR-URL: #24407 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

"That particular `emit('error', ...)` is largely defensively coded and should not ever actually happen." Sounds like an assertion rather than an error event. The code in question has no test coverage because it is believed to be unreachable. Fixes: nodejs#20673 PR-URL: nodejs#24407 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

"That particular `emit('error', ...)` is largely defensively coded and should not ever actually happen." Sounds like an assertion rather than an error event. The code in question has no test coverage because it is believed to be unreachable. Fixes: #20673 PR-URL: #24407 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

nodejs-github-bot added dont-land-on-v6.x http2 Issues or PRs related to the http2 subsystem. labels Nov 17, 2018

Trott mentioned this pull request Nov 17, 2018

http2 compat request can emit 'error' #20673

Closed

mcollina approved these changes Nov 17, 2018

View reviewed changes

jasnell approved these changes Nov 17, 2018

View reviewed changes

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

cjihrig approved these changes Nov 18, 2018

View reviewed changes

trivikr approved these changes Nov 18, 2018

View reviewed changes

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

Trott closed this Nov 20, 2018

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

codebytere mentioned this pull request Jan 15, 2019

v10.15.1 proposal #25346

Merged

Trott deleted the unreachable branch January 13, 2022 22:50

Uh oh!

Conversation

Trott commented Nov 17, 2018

Checklist

Uh oh!

nodejs-github-bot commented Nov 17, 2018

Uh oh!

Trott commented Nov 17, 2018

Uh oh!

Trott commented Nov 17, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

targos commented Nov 17, 2018

Uh oh!

Trott commented Nov 18, 2018

Uh oh!

targos commented Nov 18, 2018

Uh oh!

Trott commented Nov 18, 2018

Uh oh!

targos commented Nov 18, 2018

Uh oh!

Trott commented Nov 19, 2018

Uh oh!

Trott commented Nov 20, 2018

Uh oh!

refack commented Nov 20, 2018

Uh oh!

refack commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Trott commented Nov 20, 2018

Uh oh!

Trott commented Nov 20, 2018

Uh oh!

refack commented Nov 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Nov 20, 2018

Uh oh!

Trott commented Nov 20, 2018

Uh oh!

Trott commented Nov 20, 2018

Uh oh!

refack commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

refack commented Nov 20, 2018 •

edited

Loading

refack commented Nov 20, 2018 •

edited

Loading

refack commented Nov 21, 2018 •

edited

Loading