Napi handle scope mismatch#17161

neta-kedem · 2017-11-20T21:01:00Z

#goodness_squad

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

TimothyGu

Congrats on your first contribution! Left a few comments, but other than those it should be good to go.

TimothyGu · 2017-11-20T21:12:12Z

tools/doc/node_modules/js-yaml/index.js

-);

-module.exports=require(realJSYaml);
+module.exports=yaml;


Stray change.

This is happening when you run make doc … we really should get rid of these npm installs :(

Or .gitignore them, everyone making a PR yesterday ran into this

If I'm not totally mistaken, adding an already tracked file to .gitignore has no effect 😞 . Something like git update-index --assume-unchanged would work, but every developer has to run that locally, not really a solution for this problem.

TimothyGu · 2017-11-20T21:12:59Z

src/node_api.cc

 "The async work item was cancelled",
-"napi_escape_handle already called on scope"};
+"napi_escape_handle already called on scope",
+"invalid handle scope usage"};


Capitalize "invalid" in accordance with the rest of error_messages.
The one immediately before this one is an exception, as napi_escape_handle is the name of a function, and function names are case-sensitive.

benjamingr

Thanks for doing this :)

refack · 2017-11-20T21:18:35Z

Hello @neta-kedem and תודה רבה.

refack · 2017-11-20T21:21:01Z

@neta-kedem You might want to set you local git Name & e-mail according to https://help.github.com/articles/setting-your-commit-email-address-in-git/ so that GitHub will recognize that these commits are yours.

neta-kedem · 2017-11-20T21:34:29Z

Thanks! I set them correctly and amended

…de into napi_handle_scope_mismatch

TimothyGu · 2017-11-20T23:47:02Z

#17161 (comment) still seems to be a problem after the latest amendment...

cjihrig

Changes to src/node_api.cc LGTM

mhdawson

LGTM once stray change to tools/doc/node_modules/js-yaml/index.js is removed

neta-kedem · 2017-11-21T20:01:40Z

I undid the changes in the node_modules index file

aqrln · 2017-11-22T08:56:47Z

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

mhdawson · 2017-11-22T14:45:03Z

Neither of those tests should be affected by this change so believe we are good to land.

mhdawson · 2017-11-22T15:06:07Z

In the process of landing

mhdawson · 2017-11-22T15:11:06Z

Landed as a4a9a3d

PR-URL: #17161 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>

PR-URL: nodejs#17161 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>

Backport-PR-URL: #19447 PR-URL: #17161 Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>

neta added 2 commits November 20, 2017 22:39

src: updating status message list for napi_handle_scope_mismatch
178bb51

src: fixing the size of error_messages to last_error+1
0301123

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 20, 2017

neta-kedem mentioned this pull request Nov 20, 2017
Missing status message list update for napi_handle_scope_mismatch #16973
Closed

TimothyGu approved these changes Nov 20, 2017
View reviewed changes

benjamingr approved these changes Nov 20, 2017
View reviewed changes

refack approved these changes Nov 20, 2017
View reviewed changes

refack added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Nov 20, 2017

src: capitalize error message- Invalid handle scope usage
ad4bd24

src: capitalize error message- Invalid handle scope usage
9bbfe39

neta-kedem added 3 commits November 20, 2017 23:41

Merge branch 'napi_handle_scope_mismatch' of github.com:neta-kedem/no…
3ec5489
…de into napi_handle_scope_mismatch

set email and username
0c70186

set email and username
cd89655

addaleax approved these changes Nov 20, 2017
View reviewed changes

cjihrig approved these changes Nov 21, 2017
View reviewed changes

jasnell approved these changes Nov 21, 2017
View reviewed changes

mhdawson approved these changes Nov 21, 2017
View reviewed changes

tools: undid the change in the index file
774ddd1

neta-kedem closed this Nov 21, 2017

neta-kedem reopened this Nov 21, 2017

aqrln approved these changes Nov 22, 2017
View reviewed changes

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

fhinkel approved these changes Nov 22, 2017
View reviewed changes

mhdawson self-assigned this Nov 22, 2017

mhdawson closed this Nov 22, 2017

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

MylesBorins mentioned this pull request Dec 12, 2017
v9.3.0 proposal #17631
Merged

MylesBorins added the dont-land-on-v6.x label Dec 19, 2017

gibfahn added the land-on-v8.x label Dec 19, 2017

gibfahn mentioned this pull request Dec 20, 2017
v8.9.4 proposal #17772
Closed

gibfahn mentioned this pull request Dec 20, 2017
v8.9.4 proposal #17774
Merged

MylesBorins mentioned this pull request Apr 16, 2018
v6.14.2 proposal #19996
Merged

Uh oh!

Napi handle scope mismatch#17161

Napi handle scope mismatch #17161

Uh oh!

Conversation

neta-kedem commented Nov 20, 2017

Checklist

Affected core subsystem(s)

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGuNov 20, 2017

Choose a reason for hiding this comment

Uh oh!

addaleaxNov 20, 2017

Choose a reason for hiding this comment

Uh oh!

benjamingrNov 21, 2017

Choose a reason for hiding this comment

Uh oh!

fhinkelNov 21, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGuNov 20, 2017

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Nov 20, 2017

Uh oh!

refack commented Nov 20, 2017

Uh oh!

neta-kedem commented Nov 20, 2017

Uh oh!

TimothyGu commented Nov 20, 2017

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

neta-kedem commented Nov 21, 2017

Uh oh!

aqrln commented Nov 22, 2017

Uh oh!

mhdawson commented Nov 22, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Nov 22, 2017

Uh oh!

mhdawson commented Nov 22, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

mhdawson commented Nov 22, 2017•
edited
Loading