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

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

Conversation

gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Mar 9, 2018

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

@nodejs-github-bot 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 gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Mar 9, 2018
@jasnell jasnell changed the title n-api: backport changes from master [8.x] n-api: backport changes from master Mar 9, 2018
@gabrielschulhof gabrielschulhof force-pushed the v8.x-backport-n-api-updated-squashed branch 3 times, most recently from d26ba32 to afcc935 Compare March 16, 2018 16:40
@gabrielschulhof gabrielschulhof force-pushed the v8.x-backport-n-api-updated-squashed branch 3 times, most recently from b4c9b34 to b1223b3 Compare March 19, 2018 15:49
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Mar 19, 2018

Fixes nodejs/abi-stable-node#297.

@gibfahn
Copy link
Member

gibfahn commented Mar 20, 2018

@gabrielschulhof
Copy link
Contributor Author

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

@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 21, 2018

@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
Copy link
Member

gibfahn commented Mar 22, 2018

@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
Copy link
Contributor Author

gabrielschulhof commented Mar 22, 2018 via email

@gabrielschulhof
Copy link
Contributor Author

@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 gabrielschulhof force-pushed the v8.x-backport-n-api-updated-squashed branch from b1223b3 to c0b089c Compare March 22, 2018 16:58
@gabrielschulhof
Copy link
Contributor Author

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

@gabrielschulhof
Copy link
Contributor Author

@gabrielschulhof
Copy link
Contributor Author

The same unrelated failure has reoccurred.

@gibfahn
Copy link
Member

gibfahn commented Mar 23, 2018

@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
Copy link
Contributor Author

@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 gabrielschulhof force-pushed the v8.x-backport-n-api-updated-squashed branch from c0b089c to 7ea7a32 Compare March 28, 2018 19:56
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Mar 28, 2018

@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
Copy link
Contributor Author

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

@addaleax
Copy link
Member

@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 :)

MylesBorins pushed a commit that referenced this pull request May 1, 2018
Backport-PR-URL: #19265
PR-URL: #18498
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
Backport-PR-URL: #19265
PR-URL: #18581
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
Missing the length argument in napi_create_function.

Backport-PR-URL: #19265
PR-URL: #18661
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
Backport-PR-URL: #19265
PR-URL: #18697
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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: #18719
Fixes: #18702
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
Passing a pointer to a static integer is sufficient for the test.

Backport-PR-URL: #19265
PR-URL: #19039
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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: #19073
Fixes: #19071
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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 <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
Added a N-API test to verify new.target behavior.

Backport-PR-URL: #19265
PR-URL: #19236
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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 <cjihrig@gmail.com>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
Backport-PR-URL: #19265
PR-URL: #19385
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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: #19362
Fixes: nodejs/node-addon-api#235
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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 <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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 <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
Backport-PR-URL: #19265
PR-URL: #19555
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
Bump the version due to additions to the api.

Backport-PR-URL: #19265
PR-URL: #19497
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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 <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
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 <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 1, 2018
* 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 <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

MylesBorins commented May 1, 2018

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 MylesBorins closed this May 1, 2018
@gabrielschulhof gabrielschulhof deleted the v8.x-backport-n-api-updated-squashed branch May 2, 2018 13:11
@MylesBorins
Copy link
Contributor

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
Copy link
Contributor Author

gabrielschulhof commented May 3, 2018

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

I used

git log --oneline 3225601...07a6770 | \
while read 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
Copy link
Contributor

@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
Copy link
Contributor

gabrielf commented May 3, 2018

@MylesBorins you are asking the wrong Gabriel...

@gabrielschulhof
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. node-api Issues and PRs related to the Node-API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.