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

http2: replace unreachable error with assertion #24407

Closed
wants to merge 1 commit into from

Conversation

@Trott
Copy link
Member

commented Nov 17, 2018

"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
http2: replace unreachable error with assertion
"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
@Trott

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2018

@Trott

This comment has been minimized.

Copy link
Member Author

commented Nov 17, 2018

@mcollina
Copy link
Member

left a comment

LGTM

@targos

This comment has been minimized.

Copy link
Member

commented Nov 17, 2018

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

@Trott

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2018

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

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

@Trott Trott added the author ready label Nov 18, 2018

@targos

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

Yes. CHECK macros were added in #18852

@Trott

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2018

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

This comment has been minimized.

Copy link
Member

commented Nov 18, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Nov 19, 2018

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 Trott removed the author ready label Nov 19, 2018

@Trott

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2018

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

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

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

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

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2018

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

@refack

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

(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

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Nov 20, 2018

Landed in 566a694

@Trott Trott closed this Nov 20, 2018

Trott added a commit to Trott/io.js that referenced this pull request Nov 20, 2018
http2: replace unreachable error with assertion
"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

This comment has been minimized.

Copy link
Member

commented Nov 21, 2018

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.

targos added a commit that referenced this pull request Nov 21, 2018
http2: replace unreachable error with assertion
"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>
rvagg added a commit that referenced this pull request Nov 28, 2018
http2: replace unreachable error with assertion
"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>
@BridgeAR BridgeAR referenced this pull request Dec 5, 2018
4 of 4 tasks complete
codebytere added a commit that referenced this pull request Jan 13, 2019
http2: replace unreachable error with assertion
"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>
refack added a commit to refack/node that referenced this pull request Jan 14, 2019
http2: replace unreachable error with assertion
"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>
@codebytere codebytere referenced this pull request Jan 15, 2019
MylesBorins added a commit that referenced this pull request Jan 29, 2019
http2: replace unreachable error with assertion
"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>
codebytere added a commit that referenced this pull request Jan 29, 2019
http2: replace unreachable error with assertion
"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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.