[8.x] n-api: backport changes from master#19265

gabrielschulhof · 2018-03-09T18:42:36Z

The following commits (or relevant portions thereof) are included: * c123467 timers: allow Immediates to be unrefed (the portion fixing the uv_test_loop test) * a555397 n-api: add methods to open/close callback scope * 47f664e n-api: wrap control flow macro in do/while * 1286923 n-api: implement wrapping using private properties * 316b5ef n-api: throw RangeError napi_create_typedarray() * 8938c4c n-api: expose n-api version in process.versions * 91c1ccd n-api: throw RangeError in napi_create_dataview() with invalid range * 94e2951 n-api: fix memory leak in napi_async_destroy() * dc389bf n-api: use nullptr instead of NULL in node_api.cc * a4a9a3d src: add napi_handle_scope_mismatch to msg list

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

gabrielschulhof · 2018-03-19T15:51:42Z

Fixes nodejs/abi-stable-node#297.

gibfahn · 2018-03-20T17:30:14Z

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

gabrielschulhof · 2018-03-20T20:23:53Z

This also fails on an unrelated test: parallel/test-benchmark-misc.

MylesBorins · 2018-03-21T00:20:03Z

@gabrielschulhof we have generally backported every commit individually... that makes it easier to audit in the future and stop tooling from breaking

@nodejs/lts thoughts on landing a single commit comprising of many patches?

gibfahn · 2018-03-22T15:44:42Z

@nodejs/lts thoughts on landing a single commit comprising of many patches?

I'd rather backport each commit individually, although I'm +1 on it being in one PR (i.e. this one). @gabrielschulhof if you could split this into the individual commits that would be good. You don't need to change any commit metadata, so it should just be a case of cherry-picking and fixing merge conflicts.

gabrielschulhof · 2018-03-22T16:34:50Z

Can do.

…

On Thu, Mar 22, 2018 at 11:45 AM, Gibson Fahnestock < ***@***.***> wrote: @nodejs/lts <https://github.com/orgs/nodejs/teams/lts> thoughts on landing a single commit comprising of many patches? I'd rather backport each commit individually, although I'm +1 on it being in one PR (i.e. this one). @gabrielschulhof <https://github.com/gabrielschulhof> if you could split this into the individual commits that would be good. You don't need to change any commit metadata, so it should just be a case of cherry-picking and fixing merge conflicts. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19265 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA7k0UYxc8Ju-XtmJ8yYplmjJ0YArQK-ks5tg8c0gaJpZM4Sksat> .

gabrielschulhof · 2018-03-22T16:57:16Z

@gibfahn there are some commits where I've intentionally not applied parts of it even though they would've applied cleanly, because those parts were not related to N-API.

gabrielschulhof · 2018-03-22T17:00:37Z

@gibfahn @MylesBorins I have now replaced the single commit with the sometimes-partially cherry-picked individual commits.

gabrielschulhof · 2018-03-22T17:01:51Z

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

gabrielschulhof · 2018-03-22T21:06:51Z

The same unrelated failure has reoccurred.

gibfahn · 2018-03-23T11:54:59Z

@gibfahn there are some commits where I've intentionally not applied parts of it even though they would've applied cleanly, because those parts were not related to N-API.

So if you think those changes should be applied to 8.x then you should leave them in, we try to backport whole commits where possible, otherwise understanding what's been backported becomes even harder.

If they don't need to go back to 8.x at all, then leaving them out is fine.

Sorry to keep complaining about this PR, I really appreciate you doing this work!

gabrielschulhof · 2018-03-23T13:08:47Z

@gibfahn NP. We have to do things right.

I'll do the cherry-picking again, being more inclusive. Any decisions I make I will document in a subsequent comment here.

gabrielschulhof · 2018-03-28T20:32:59Z

@MylesBorins @gibfahn the list of commits from master:

⁠	⁠	⁠	⁠	⁠	⁠
`6572f63`	`d4b3fcb`	`c6c957d`	`be47094`	`c1695d8`	`f24d0ec`
`6a5a9ad`	`d20f1f0`	`5fb6f7f`	`cd7d7b1`	`a03c90b`	`33e63fe`
`48b5c11`	`35c7238`	`d83f830`	`449d1c8`	`c698017`	`a29089d`
`9cb96ac`	`6e1c25c`	`463d1a4`	`caee112`	`755e07c`	`a1bab82`
`2bead4b`	`1729af2`	`fe442f6`	`d3569b6`	`5ecd2e1`	`6101bd2`
`d379db3`	`a555397`	`c2b9048`	`629a447`	`47f664e`	`1286923`
`0c16b18`	`6e312c5`	`6c1906a`	`c123467`	`f054855`	`c90185c`
`316b5ef`	`f75bc2c`	`3e06276`	`8938c4c`	`91c1ccd`	`8a86d9c`
`93acfe5`	`094d92b`	`94e2951`	`246aeac`	`ef49f55`

These are the places where I made a judgement call:

c123467 timers: allow Immediates to be unrefed
I did not backport the immediate.ref() and immediate.unref(), but only the modification to test/addons-napi/test_uv_loop/test_uv_loop.cc
d3569b6 doc: remove **Note:** tags
This touched N-API, so I grabbed the whole thing, dropping chunks which do not apply, and manually applying the fix in places where there are such tags that were not on master
caee112 test: remove assert.doesNotThrow()
I removed the files that were "deleted by us", checked out "our" version of those that conflicted, and removed assert.doesNotThrow() by hand
463d1a4 test,benchmark,doc: enable dot-notation rule
I removed the files that were "deleted by us", checked out "our" version of those that conflicted, and moved to dot-notation by hand. I ran the new linter to make sure everything was covered.
6e1c25c errors: update all internal errors
I removed the unused errors but I did not backport the updated definition of E() which accepts an additional parameter.
9cb96ac doc: remove extraneous "for example" text
Did my best to merge this, since its documentation-only.
a29089d doc: add new documentation lint rule
I applied as much of the commit as possible, checking out "our" version of conflicting files, and manually wrapping lines at 80 characters to conform to the linting rule.
35c7238 doc: replace to Node.js
I applied as much of the commit as possible, checking out "our" version of conflicting files, and using git grep -niH 'node' -- doc/api doc/guides tools/icu/README.md | grep -v 'Node[.]js' | grep -i --color=always node | less -R to find places that should read "Node.js" instead of "node". I found one place where it's better to replace "node" with "`node`" rather than "Node.js", because the text refers to the name of the binary.

gabrielschulhof · 2018-03-28T20:38:11Z

I'll close and open another because this needs to go on top of v8.x-staging.

addaleax · 2018-03-28T20:39:25Z

@gabrielschulhof Not sure if that’s what you mean by your last comment, but using the same button that you’d use to change a PR title, you can also change its base branch :)

Backport-PR-URL: #19265 PR-URL: #18498 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Backport-PR-URL: #19265 PR-URL: #18581 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Missing the length argument in napi_create_function. Backport-PR-URL: #19265 PR-URL: #18661 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

Backport-PR-URL: #19265 PR-URL: #18697 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

The timer in NAPI's test_callback_scope/test-resolve-async.js can be removed. If the test fails, it will timeout on its own. The extra timer increases the chances of the test being flaky. Backport-PR-URL: #19265 PR-URL: #18719Fixes: #18702 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

Passing a pointer to a static integer is sufficient for the test. Backport-PR-URL: #19265 PR-URL: #19039 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>

Documentation for N-API Custom Asynchronous Operations incorrectly stated that async execution happens on the main event loop. Added details to napi_create_async_work about which threads are used to invoke the execute and complete callbacks. Changed 'async' to 'asynchronous' in the documentation for Custom Asynchronous Operations. Changed "executes in parallel" to "can execute in parallel" for the documentation of napi_create_async_work execute parameter. Backport-PR-URL: #19265 PR-URL: #19073Fixes: #19071 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Remove the necessity for allocating on the heap, and assert that the correct pointer gets passed to the finalizer. Backport-PR-URL: #19265 PR-URL: #19086 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Document which APIs work while an exception is pending. This is the list of all APIs which do not start with `NAPI_PREAMBLE(env)`. Fixes: nodejs/abi-stable-node#257 Backport-PR-URL: #19265 PR-URL: #19078 Reviewed-By: Michael Dawson <[email protected]>

The last promise created by the test for the purposes of making sure that its type is indeed a promise needs to be resolved so as to avoid having it left in the pending state at the end of the test. Backport-PR-URL: #19265 PR-URL: #19245 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Added a N-API test to verify new.target behavior. Backport-PR-URL: #19265 PR-URL: #19236 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Added some simple tests to verify that the int64 API is correctly handling numbers greater than 32-bits. This is a basic test, but verifies that an implementer hasn't truncated back to 32-bits. Refs: nodejs/node-chakracore#496 Backport-PR-URL: #19265 PR-URL: #19309 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: James M Snell <[email protected]>

Backport-PR-URL: #19265 PR-URL: #19385 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Add checks for a pending exception in napi_make_callback after the callback has been invoked. If there is a pending exception then we need to avoid checking the result as that will not be able to complete properly. Add additional checks to the unit test for napi_make_callback to catch this case. Backport-PR-URL: #19265 PR-URL: #19362Fixes: nodejs/node-addon-api#235 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Place the test_make_callback async_hooks-related test into its own file. Backport-PR-URL: #19265 PR-URL: #19392 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Add function to trigger and uncaught exception. Useful if an async callback throws an exception with no way to recover. Backport-PR-URL: #19265 PR-URL: #19337 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

This re-writes the test in C by dropping std::vector<napi_value> in favour of a C99 variable length array, and by dropping the anonymous namespace in favour of static function declarations. Backport-PR-URL: #19265 PR-URL: #19448 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Backport-PR-URL: #19265 PR-URL: #19555 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

Bump the version due to additions to the api. Backport-PR-URL: #19265 PR-URL: #19497 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Whenever we call into an addon, whether it is for a callback, for module init, or for async work-related reasons, we should make sure that * the last error is cleared, * the scopes before the call are the same as after, and * if an exception was thrown and captured inside the module, then it is re-thrown after the call. Therefore we should call into the module in a unified fashion. This change introduces the macro NAPI_CALL_INTO_MODULE() which should be used whenever invoking a callback provided by the module. Fixes: #19437 Backport-PR-URL: #19265 PR-URL: #19537 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

Heed the comment to not use fields of a Reference after calling its finalize callback, because such a call may destroy the Reference. Fixes: #19673 Backport-PR-URL: #19265 PR-URL: #19718 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

* Updated tests for `Number` and `int32_t` * Added new tests for `int64_t` * Updated N-API `int64_t` behavior to return zero for all non-finite numbers * Clarified the documentation for these calls. Backport-PR-URL: #19265 PR-URL: #19402 Refs: nodejs/node-chakracore#500 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>

MylesBorins · 2018-05-01T21:19:27Z

landed in 3225601...07a6770

edit: automated adding the backport-pr-url labels with the following command

gitfilter-branch --msg-filter 'perl -pe "s/PR-URL/Backport-PR-URL: https://github.com//pull/19265\nPR-URL/g"' upstream/v8.x-staging..v8.x-staging

MylesBorins · 2018-05-02T20:18:36Z

It seems like e15f577 was only a partial backport of a test from a semver-minor commit... this will make it impossible for us to audit that semver-minor commit in the future if we wanted to land it.

Were there any other commits / prs that were partially backported?

gabrielschulhof · 2018-05-03T01:38:04Z

@MylesBorins 2838f9b doesn't modify test/parallel/test-console-assign-undefined.js whereas the original does.

I used

git log --oneline 3225601...07a6770 | \ whileread id message;do \ echo$id; \ diff -u \ <(git show --stat=99 $id| grep -vE '^commit|files changed'| grep '|') \ <(git log --oneline master | grep "$message"| awk '{print $1}'| xargs \ git show --stat=99 | grep -vE '^commit|files changed'| grep '|'); \ done

to check whether the files touched by the backported commits differ from the files touched by the commits on master.

MylesBorins · 2018-05-03T18:11:45Z

@gabrielf did that script identify e15f577 as well? If so this satisfied me!

Perhaps we should consider putting this into node-core-utils to help us with reviewing backports /cc @nodejs/automation

gabrielf · 2018-05-03T19:11:35Z

@MylesBorins you are asking the wrong Gabriel...

gabrielschulhof · 2018-05-03T20:02:04Z

@gabrielf sorry!

@MylesBorins it did. This was the output:

07a6770614 8b3ef4660a 92f699e021 367113f5d7 f0ba2c6ceb 24b8bb6708 3a6b7e610d 9949d55ae9 f29d8e0e8d 7c6fa183cb 584fadc605 4c1181dc02 faf94b1c49 df63adf7aa b26410e86f 1abb168838 cb3f90a1a9 6129ff4b99 87d0fd8212 58688d97dc 2838f9b150 --- /dev/fd/63 2018-05-03 15:57:59.614213570 -0400 +++ /dev/fd/62 2018-05-03 15:57:59.614213570 -0400 @@ -1 +1,2 @@ - test/addons-napi/test_typedarray/test.js | 4 ++-- + test/addons-napi/test_typedarray/test.js | 4 ++-- + test/parallel/test-console-assign-undefined.js | 3 +-- 5c0983e5a2 4d43607474 --- /dev/fd/63 2018-05-03 15:58:00.621202048 -0400 +++ /dev/fd/62 2018-05-03 15:58:00.621202048 -0400 @@ -1 +1,2 @@ doc/api/n-api.md | 2 +- + doc/api/n-api.md | 2 +- 9244e1d234 927fc0b19f 9729278007 7ed1dfef28 969a520990 d89f5937eb bebcdfe382 af655f586c 5b1b74c5a5 e15f57745d --- /dev/fd/63 2018-05-03 15:58:05.856142148 -0400 +++ /dev/fd/62 2018-05-03 15:58:05.856142148 -0400 @@ -1 +1,10 @@ - test/addons-napi/test_uv_loop/test_uv_loop.cc | 9 +++++++++ + doc/api/timers.md | 32 +++++++++ + lib/timers.js | 143 +++++++++++++++++++++++---------------- + src/env-inl.h | 40 +++++++++-- + src/env.cc | 36 +++++----- + src/env.h | 17 ++++- + src/node_perf.cc | 18 ++--- + src/timer_wrap.cc | 12 ++-- + test/addons-napi/test_uv_loop/test_uv_loop.cc | 9 +++ + test/parallel/test-timers-immediate-unref-simple.js | 7 ++ + test/parallel/test-timers-immediate-unref.js | 37 ++++++++++ 84e0a03727 8cfa87832d ca10fda064 51513fdf9e 02ae3295d5 853b4d593c 48be8a4793 79ecc2c586 ad8c079af7 a744535f99 2e100c82be 077e1870ae

I didn't mention 4d43607 because there are two commits with the same message on master.

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. v8.x labels Mar 9, 2018

gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Mar 9, 2018

jasnell changed the title ~~n-api: backport changes from master~~[8.x] n-api: backport changes from masterMar 9, 2018

gabrielschulhof force-pushed the v8.x-backport-n-api-updated-squashed branch 3 times, most recently from d26ba32 to afcc935Compare March 16, 2018 16:40

gabrielschulhof force-pushed the v8.x-backport-n-api-updated-squashed branch 3 times, most recently from b4c9b34 to b1223b3Compare March 19, 2018 15:49

gabrielschulhof mentioned this pull request Mar 20, 2018
n-api: implement wrapping using private properties #18311
Closed
2 tasks

gabrielschulhof force-pushed the v8.x-backport-n-api-updated-squashed branch from b1223b3 to c0b089cCompare March 22, 2018 16:58

gabrielschulhof force-pushed the v8.x-backport-n-api-updated-squashed branch from c0b089c to 7ea7a32Compare March 28, 2018 19:56

gabrielschulhof closed this Mar 28, 2018

MylesBorins pushed a commit that referenced this pull request May 1, 2018
doc: Add a missing comma
f0ba2c6
Backport-PR-URL: #19265 PR-URL: #19555 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

MylesBorins closed this May 1, 2018

gabrielschulhof deleted the v8.x-backport-n-api-updated-squashed branch May 2, 2018 13:11

Uh oh!

[8.x] n-api: backport changes from master#19265

[8.x] n-api: backport changes from master #19265

Uh oh!

Conversation

gabrielschulhof commented Mar 9, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

gabrielschulhof commented Mar 19, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn commented Mar 20, 2018

Uh oh!

gabrielschulhof commented Mar 20, 2018

Uh oh!

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

Uh oh!

Uh oh!

gibfahn commented Mar 22, 2018

Uh oh!

gabrielschulhof commented Mar 22, 2018 via email

Uh oh!

gabrielschulhof commented Mar 22, 2018

Uh oh!

gabrielschulhof commented Mar 22, 2018

Uh oh!

gabrielschulhof commented Mar 22, 2018

Uh oh!

gabrielschulhof commented Mar 22, 2018

Uh oh!

gibfahn commented Mar 23, 2018

Uh oh!

gabrielschulhof commented Mar 23, 2018

Uh oh!

gabrielschulhof commented Mar 28, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gabrielschulhof commented Mar 28, 2018

Uh oh!

addaleax commented Mar 28, 2018

Uh oh!

MylesBorins commented May 1, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented May 2, 2018

Uh oh!

gabrielschulhof commented May 3, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MylesBorins commented May 3, 2018

Uh oh!

gabrielf commented May 3, 2018

Uh oh!

gabrielschulhof commented May 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

gabrielschulhof commented Mar 9, 2018•
edited
Loading

gabrielschulhof commented Mar 19, 2018•
edited
Loading

MylesBorins commented Mar 21, 2018•
edited
Loading

gabrielschulhof commented Mar 28, 2018•
edited
Loading

MylesBorins commented May 1, 2018•
edited
Loading

gabrielschulhof commented May 3, 2018•
edited
Loading