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.

Sign up for GitHub

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

Jump to bottom

src: handle empty Maybe(Local) in node_util.cc #33867

Closed
wants to merge 3 commits into from
Closed

src: handle empty Maybe(Local) in node_util.cc #33867

wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jun 13, 2020 •
edited
Loading

Refs: https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion

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

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module. labels Jun 13, 2020
bnoordhuis
bnoordhuis reviewed Jun 14, 2020
View reviewed changes
src/node_util.cc Show resolved Hide resolved
src/node_util.cc
if (!args[1]->Uint32Value(env->context()).To(&index)) return;
Local<Private> private_symbol = IndexToPrivateSymbol(env, index);
Local<Value> ret;
if (obj->GetPrivate(env->context(), private_symbol).ToLocal(&ret))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not 100% sure but I don't think GetPrivate() can actually fail provided that obj->IsObject().

@addaleax
Copy link
Member Author

@bnoordhuis The point isn’t so much as to whether these ops can currently fail or not, it’s more that I want to move more of our code into a state where it is “obviously not broken”, and be consistent about that. At least the SetPrivate() call can fail, and if we didn’t use .ToLocalChecked() & friends as excessively as we currently do, we probably wouldn’t have used it for that call either.

@bnoordhuis
Copy link
Member

We'll have to disagree on what "obviously not broken" means.

Replacing CHECK(args[1]->IsUint32()) with a (recoverable) JS exception just makes it harder to diagnose bugs, IMO.

Point ceded on the SetPrivate() call, although I'd still like people to report it when it fails, because that's likely a bug we need to fix.

(You can probably trigger it if you go out of your way to poison the runtime but normally I would expect it to be a bug on our side.)

@addaleax
Copy link
Member Author

Point ceded on the SetPrivate() call, although I'd still like people to report it when it fails, because that's likely a bug we need to fix.

Like a lot of operations, it fails when JS execution is being terminated. That’s one of the main points here – it is only obvious that .ToLocalChecked() doesn’t fail in very few cases, so it’s better to avoid it as much as possible.

@bnoordhuis
Copy link
Member

If TerminateExecution() compatibility is the goal here, why don't you mention that in the commit log? That would have avoided this back and forth.

My point about changing the CHECK remains though: it serves no point that I can see.

@addaleax
Copy link
Member Author

If TerminateExecution() compatibility is the goal here, why don't you mention that in the commit log? That would have avoided this back and forth.

Because I thought it was obvious, sorry. I’ll add Refs: https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion for PRs like this in the future.

My point about changing the CHECK remains though: it serves no point that I can see.

Okay, since you feel strongly about it, I’ve restored it, and switched to the variant that we use when we CHECK() the type (i.e. .As<Uint32>()->Value() instead of ->Uint32Value(…)), as consistency is half of the point of this PR.

bnoordhuis
bnoordhuis approved these changes Jun 15, 2020
View reviewed changes
src/node_util.cc Show resolved Hide resolved
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 19, 2020
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Jun 20, 2020
Open
Open
Open
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Jun 25, 2020
Open
14 tasks
jasnell pushed a commit that referenced this pull request Jun 25, 2020
@addaleax @jasnell
src: handle empty Maybe(Local) in node_util.cc
8e12962 
PR-URL: #33867
Refs: https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jun 25, 2020

Landed in 8e12962

@jasnell jasnell closed this Jun 25, 2020
codebytere pushed a commit that referenced this pull request Jun 27, 2020
@addaleax @codebytere
src: handle empty Maybe(Local) in node_util.cc
9a52cd9 
PR-URL: #33867
Refs: https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jun 28, 2020
Merged
codebytere pushed a commit that referenced this pull request Jun 30, 2020
@addaleax @codebytere
src: handle empty Maybe(Local) in node_util.cc
8a6a644 
PR-URL: #33867
Refs: https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Jul 10, 2020
@addaleax @codebytere
src: handle empty Maybe(Local) in node_util.cc
1ad73dd 
PR-URL: #33867
Refs: https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Jul 12, 2020
@addaleax @codebytere
src: handle empty Maybe(Local) in node_util.cc
153f292 
PR-URL: #33867
Refs: https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Jul 13, 2020
Merged
codebytere pushed a commit that referenced this pull request Jul 14, 2020
@addaleax @codebytere
src: handle empty Maybe(Local) in node_util.cc
46dce9b 
PR-URL: #33867
Refs: https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Lusito Lusito mentioned this pull request Nov 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@addaleax @bnoordhuis @nodejs-github-bot @jasnell @cjihrig