Napi handle scope mismatch by neta-kedem · Pull Request #17161 · nodejs/node

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 <timothygu99@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

PR-URL: nodejs#17161 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Backport-PR-URL: #19447 PR-URL: #17161 Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

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!

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!

TimothyGu Nov 20, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Nov 20, 2017

Choose a reason for hiding this comment

Uh oh!

benjamingr Nov 21, 2017

Choose a reason for hiding this comment

Uh oh!

fhinkel Nov 21, 2017

Choose a reason for hiding this comment

Uh oh!

TimothyGu Nov 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