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

[6.x] n-api: backport to v6.x #19447

Closed
wants to merge 183 commits into from
Closed

[6.x] n-api: backport to v6.x #19447

wants to merge 183 commits into from

Conversation

gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Mar 19, 2018 •
edited
Loading

  • src/node_api_backport.cc and src/node_api_backport.h are added
    containing stubs for internal APIs used by N-API.

  • expectsError() is brought in from master and added to
    tests/common/index.js.

  • N-API tests involving the JS async_hooks module are removed.

  • The following commits are partially or completely applied:

f24d0ec 6a5a9ad d20f1f0 5fb6f7f cd7d7b1 a03c90b
33e63fe 48b5c11 35c7238 d83f830 449d1c8 c698017
a29089d 9cb96ac 945eb1b 6e1c25c 463d1a4 caee112
755e07c a1bab82 2bead4b 1729af2 fe442f6 d3569b6
5ecd2e1 6101bd2 d379db3 a555397 c2b9048 629a447
47f664e 1286923 0c16b18 6e312c5 6c1906a c123467
f054855 f7c709f c90185c 316b5ef f75bc2c 3e06276
8938c4c 91c1ccd 8a86d9c 93acfe5 094d92b 94e2951
246aeac a893e79 ef49f55 d4cd8c2 50afd90 2475676
ff9a6bc 12c8b4d dc389bf 493340f 8f2c366 a4a9a3d
9cf3525 97ba69f 959c425 9b4ab14 51f92b6 e0b1394
3ee524b f2cb78c e04d23c 2336df1 0736ad4 201ecef
e07e708 7be4a84 b3f9b38 6bc82da a1c0804 f881789
3594223 17b818b 4957726 b33b3e1 84579b1 e6e58fd
cec6e21 05e4c1d 8b90250 a406a32 3070d53 a8c0a43
1976654 973c12f 0c258bd 92e5f5c 1a0727d 8c8c90b
290315a cb94905 a10856a 61e9ba1 7828698 1cdb41f
c77e6d3 7efb8f7 6c382de 82bad0b 70664bf e96b949
1fe0741 85d7d97 e6eb5c0 835c383 b72e702 af70c3b
ad664ea f3afe29 6968ead aa6fac6 77ca3cb 9926dfe
7849b52 e59987c 57a4ceb 4d1e086 bb29405 65eefa0
e3f7a54 73078d6 ac41db4 598a128 f52c707 d5b397c
8f3dab4 9c6804c 29df1a8 34cf8ad e36917b c9b6d95
ea927b3 732ae41 f803e77 0f1888f a71d205 ef28d85
3e18c49 d291338 ecf6a46 62e940d ca8a29c 2529119
01f4d9a c28418a df46fcb 7d9dfda ddba969 bd4b790
062071a 7a7ac1c 8ab8c33 fd54b10 bb91879 43e4efd
effeff1 d9ee297 1961900 2af49b6 260cd41 47919b3
f3ef971 bfade5a 4a7b7e8 a63b245 0dd8b9a 0083011
47c3c58 1b28022 9516aa1 abfd4bf 654afa2 147048a
2e3fef7 2bbabb1 0a734fe 73d9c0f 94a120c 8aca66a
cd32b77 a180259 0142276 deb9622 972bfe1 4271254
1d96803 4241577 b7a341d ba7bac5 468275a 6c60691
70b51c8 9d52222 46f2026 ad5f987 8bd26d3 9de2e15
0ec0272 affe0f2 9decfb1 ca786c3 afd5966 8fbace1
8460284 0a5bf4a 491d59d 4a21e39 56e881d

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. v6.x labels Mar 19, 2018
@gabrielschulhof gabrielschulhof force-pushed the v6.x-backport-n-api branch 2 times, most recently from 95aece3 to 17f4970 Compare March 19, 2018 15:49
@gabrielschulhof
Copy link
Contributor Author

Fixes nodejs/abi-stable-node#298.

@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Mar 19, 2018
@MylesBorins MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 20, 2018
@MylesBorins
Copy link
Contributor

@gabrielschulhof great work. Is this landed with the experimental flag in place? We may want to consider doing a semver minor for this... not 100%. We were not planning another minor for 6.x before it went into maintenance mode though

/cc @nodejs/lts, thoughts?

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Mar 20, 2018 via email

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Mar 20, 2018 via email

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Mar 20, 2018 via email

@MylesBorins
Copy link
Contributor

There is another PR that was just opened that would be semver minor against 6.x as well

I'll open an issue in the release repo tomorrow to discuss it

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Mar 20, 2018 via email

@mhdawson
Copy link
Member

@gabrielschulhof thanks for all the hard work on this.

@gabrielschulhof
Copy link
Contributor Author

@mhdawson NP! Happy to help!

@gabrielschulhof
Copy link
Contributor Author

@MylesBorins I have now re-done this backport with complete commits as much as possible, much like the v8.x backport.

@gabrielschulhof gabrielschulhof changed the base branch from v6.x to v6.x-staging April 6, 2018 17:01
@MylesBorins
Copy link
Contributor

@nodejs/tsc @nodejs/lts if we want to land this we will have to do another semver-minor of v6.x, which I am somewhat uncomfortable with doing just before going into maintenance mode.

@mhdawson @jasnell @rvagg @addaleax @ofrobots who all may have particular thoughts here

@cjihrig
Copy link
Contributor

cjihrig commented Apr 6, 2018

I think I'm in favor. It would be nice to have N-API in all of the supported release lines.

@mcollina
Copy link
Member

mcollina commented Apr 6, 2018

I’m in favor!

@MylesBorins
Copy link
Contributor

204 commits on 569 files with less than a month before maintenance... we don't even have time to follow our process for testing a semver-minor. I'm floating between -0 and -1. Looking forward to more people chiming in

@mcollina
Copy link
Member

mcollina commented Apr 6, 2018

@gabrielschulhof I think several commits got pulled in that have absolutely no relation to n-api. As an example, the whole reformatting of the docs to 80-chars width has been pulled in, and a change in the eslint. Those would have to be removed IMHO. @MylesBorins those are the the vast majority of the changed and the touched files.

@mcollina
Copy link
Member

mcollina commented Apr 6, 2018 •
edited
Loading

Specifically 66e6d0bdcd346af488aaf3c3d383c5fe204d6c77 and 0b2d5bc3fd1025d9bef2e9934d4b898826b30236 (this one is 243 files!)

@rvagg
Copy link
Member

rvagg commented Apr 6, 2018

wow, lots of work here, +1 from me as long as you think it's fairly low risk .. I haven't reviewed the changeset but if there are any major departures to how it's been applied to 8.x+ could you let us know so we can have some idea of areas of risk?

@mcollina
Copy link
Member

mcollina commented Apr 6, 2018

wow, lots of work here, +1 from me as long as you think it's fairly low risk .. I haven't reviewed the changeset but if there are any major departures to how it's been applied to 8.x+ could you let us know so we can have some idea of areas of risk?

The vast majority of the touched files are just for linting changes, so it's very hard to review. IMHO the changes in itself is relatively low risk (as long as it passes CITGM and we can verify that n-api modules works here and in 8), as it's a new feature.

MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@MylesBorins
n-api: remove extra reference from test
516c287 
Backport-PR-URL: #19447
PR-URL: #18542
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@vsemozhetbyt @MylesBorins
doc: fix typo in n-api.md
9719b83 
Backport-PR-URL: #19447
PR-URL: #18590
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@bshankar @MylesBorins
test: improve error message output
7f37dc9 
Backport-PR-URL: #19447
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]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@jackhorton @MylesBorins
test: convert new tests to use error types
c90b77e 
Backport-PR-URL: #19447
PR-URL: #18581
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@MylesBorins
doc: fix exporting a function example
e9e5d56 
Missing the length argument in napi_create_function.

Backport-PR-URL: #19447
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]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@tniessen @MylesBorins
doc: mark NAPI_AUTO_LENGTH as code
8826f18 
Backport-PR-URL: #19447
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]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@cjihrig @MylesBorins
test: remove unnecessary timer
7c58045 
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: #19447
PR-URL: #18719
Fixes: #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]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@MylesBorins
n-api: fix object test
d246374 
Passing a pointer to a static integer is sufficient for the test.

Backport-PR-URL: #19447
PR-URL: #19039
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Hitesh Kanwathirtha <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@ebickle @MylesBorins
doc: fix n-api asynchronous threading docs
10fe65a 
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: #19447
PR-URL: #19073
Fixes: #19071
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@MylesBorins
n-api: update reference test
af62c8f 
Remove the necessity for allocating on the heap, and assert that the
correct pointer gets passed to the finalizer.

Backport-PR-URL: #19447
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]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@MylesBorins
n-api: update documentation
a8237ef 
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: #19447
PR-URL: #19078
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@MylesBorins
n-api: resolve promise in test
ce14479 
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: #19447
PR-URL: #19245
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@boingoing @MylesBorins
n-api,test: add a new.target test to addons-napi
3f6d80e 
Added a N-API test to verify new.target behavior.

Backport-PR-URL: #19447
PR-URL: #19236
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@mhdawson @MylesBorins
n-api: take n-api out of experimental
2c1190a 
Take n-api out of experimental as per:
nodejs/TSC#501

Backport-PR-URL: #19447
PR-URL: #19262
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@kfarnung @MylesBorins
n-api,test: add int64 bounds tests
aa0fb77 
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: #19447
PR-URL: #19309
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@shama @MylesBorins
doc: fix typos on n-api
b9b752e 
Backport-PR-URL: #19447
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]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@mhdawson @MylesBorins
n-api: add missing exception checking
eb29266 
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: #19447
PR-URL: #19362
Fixes: nodejs/node-addon-api#235
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@mafintosh @MylesBorins
n-api: add napi_fatal_exception
49cd4fa 
Add function to trigger and uncaught exception.
Useful if an async callback throws an exception with
no way to recover.

Backport-PR-URL: #19447
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]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@MylesBorins
n-api: re-write test_make_callback
c16a705 
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: #19447
PR-URL: #19448
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@leaves4j @MylesBorins
doc: Add a missing comma
71acb52 
Backport-PR-URL: #19447
PR-URL: #19555
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@mhdawson @MylesBorins
n-api: bump version of n-api supported
c6d0a66 
Bump the version due to additions to the api.

Backport-PR-URL: #19447
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]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@MylesBorins
n-api: ensure in-module exceptions are propagated
e6ccdfb 
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: #19447
PR-URL: #19537
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@MylesBorins
n-api: back up env before finalize
abd9fd6 
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: #19447
PR-URL: #19718
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
@kfarnung @MylesBorins
n-api: add more int64_t tests
3aa5b7d 
* 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: #19447
PR-URL: #19402
Refs: nodejs/node-chakracore#500
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

landed in e54b8e8...3aa5b7d

If anything is out of the ordinary we can find it in the release tests and open separate PRs to help fix.

Was able to automate applying the Backport-PR-URL meta data to all the commits using the following command

git filter-branch --msg-filter 'perl -pe "s/PR-URL/Backport-PR-URL: https:\/\/github.com\/nodejs\/node\/pull\/19447\nPR-URL/g"' upstream/v6.x-staging..v6.x-staging

@gabrielschulhof gabrielschulhof deleted the v6.x-backport-n-api branch April 28, 2018 16:19
MylesBorins added a commit that referenced this pull request Apr 30, 2018
@MylesBorins
2018-04-30, Version 6.14.2 'Boron' (LTS)
79592b1 
Notable Change:

* n-api:
  - n-api has been backported to v6.x. It is being landed as an experimental interface,
    and as such is landing in a Semver-Patch release. (Gabriel Schulhof)
    #19447

PR-URL: #19996
MylesBorins added a commit that referenced this pull request Apr 30, 2018
@MylesBorins
2018-04-30, Version 6.14.2 'Boron' (LTS)
c6d3b37 
Notable Change:

* n-api:
  - n-api has been backported to v6.x. It is being landed as an experimental interface,
    and as such is landing in a Semver-Patch release. (Gabriel Schulhof)
    #19447

PR-URL: #19996
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardlau richardlau richardlau left review comments

@mcollina mcollina mcollina approved these changes

@mhdawson mhdawson mhdawson approved these changes

@cjihrig cjihrig cjihrig approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

@rvagg rvagg rvagg approved these changes

Assignees

@MylesBorins MylesBorins

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.