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

[v8.x backport] Backport tsfn to v8.x #25002

Closed
wants to merge 10 commits into from
Closed

[v8.x backport] Backport tsfn to v8.x #25002

wants to merge 10 commits into from

Conversation

gabrielschulhof
Copy link
Contributor

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

Backporting ThreadSafeFunction to v8.x completes its availability across all LTS release lines. This will, in turn, allow us to mark it as stable and thereby bump the available N-API version to 4.

@nodejs-github-bot
Copy link
Collaborator

@gabrielschulhof build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1916/pipeline

@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 Dec 13, 2018
gabrielschulhof
gabrielschulhof commented Dec 13, 2018
View reviewed changes
src/node_api.cc
if (ref.IsEmpty())
return;
ref.ClearWeak();
ref.Reset();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notable difference: node::Persistent is unavailable in v8.x, so ref is a normal v8::Persistent which requires resetting upon instance destruction.

src/node_api.cc
return napi_ok;
}

uv_close(reinterpret_cast<uv_handle_t*>(&async),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notable difference: env->CloseHandle() is unavailable in v8.x so this uses plain uv_close().

src/node_api.cc
return;
}
handles_closing = true;
uv_close(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, using plain uv_close() here because there is no env->CloseHandle().

src/node_api.cc
node::ContainerOf(&ThreadSafeFunction::async,
reinterpret_cast<uv_async_t*>(handle));
v8::HandleScope scope(ts_fn->env->isolate);
uv_close(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here (wrt. using plain uv_close() because env->CloseHandle() is unavailable).

@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Dec 13, 2018
@gabrielschulhof
Copy link
Contributor Author

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

@gabrielschulhof
Copy link
Contributor Author

Re: #24249

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Dec 14, 2018 •
edited
Loading

This backport incorporates the following commits:

2b1dd6c test: mark test_threadsafe_function/test as flaky
315b1c6 n-api: add missing handle scopes
357aeaa doc: fix optional parameters in n-api.md
403df7c n-api: clean up thread-safe function
5081d96 n-api: remove idle_running from TsFn
c4de500 src: fix may be uninitialized warning in n-api
53296e8 n-api: guard against cond null dereference
a27b0a3 n-api: fix compiler warning
81f06ba n-api: add API for asynchronous functions

@MylesBorins
Copy link
Contributor

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

@MylesBorins
Copy link
Contributor

@gabrielschulhof is this functionality available on v6.x?

@pipobscure pipobscure mentioned this pull request Dec 28, 2018
Closed
@gabrielschulhof
Copy link
Contributor Author

@MylesBorins I don't believe so.

Gabriel Schulhof and others added 9 commits December 28, 2018 12:00
n-api: add API for asynchronous functions
2e15372 
Bundle a `uv_async_t`, a `uv_idle_t`, a `uv_mutex_t`, a `uv_cond_t`,
and a `v8::Persistent<v8::Function>` to make it possible to call into JS
from another thread. The API accepts a void data pointer and a callback
which will be invoked on the loop thread and which will receive the
`napi_value` representing the JavaScript function to call so as to
perform the call into JS. The callback is run inside a
`node::CallbackScope`.

A `std::queue<void*>` is used to store calls from the secondary
threads, and an idle loop is started by the `uv_async_t` callback on the
loop thread to drain the queue, calling into JS with each item.

Items can be added to the queue blockingly or non-blockingly.

The thread-safe function can be referenced or unreferenced, with the
same semantics as libuv handles.

Re: nodejs/help#1035
Re: nodejs#20964
Fixes: nodejs#13512
PR-URL: nodejs#17887
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@cjihrig
n-api: fix compiler warning
b285f1d 
private field 'async_context' is not used [-Wunused-private-field]

PR-URL: nodejs#21597
Refs: nodejs#17887
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
n-api: guard against cond null dereference
7600c04 
A condition variable is only created by the thread-safe function if the
queue size is set to something larger than zero. This adds null-checks
around the condition variable and tests for the case where the queue
size is zero.

Fixes: nodejs/help#1387
PR-URL: nodejs#21871
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@mhdawson
src: fix may be uninitialized warning in n-api
7965765 
PR-URL: nodejs#21898
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@ralphtheninja
n-api: remove idle_running from TsFn
46118be 
The idle_running member variable in TsFn is always false and can
therefore be removed.

PR-URL: nodejs#22520
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
n-api: clean up thread-safe function
35c6b1d 
* Move class `TsFn` to name space `v8impl` and rename it to
  `ThreadSafeFunction`
* Remove `NAPI_EXTERN` from API declarations, because it's only needed
  in the header file.

PR-URL: nodejs#22259
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@ralphtheninja
doc: fix optional parameters in n-api.md
8efb65b 
The thread_finalize_data and thread_finalize_cb parameters in
napi_create_threadsafe_function are optional.

PR-URL: nodejs#22998
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@danbev
n-api: add missing handle scopes
f116dc4 
Currently when building with --debug
test/addons-napi/test_threadsafe_function will error:

$  out/Debug/node test/addons-napi/test_threadsafe_function/test.js
FATAL ERROR: v8::HandleScope::CreateHandle()
  Cannot create a handle without a HandleScope
 1: 0x10004e287 node::DumpBacktrace(__sFILE*) [node/out/Debug/node]
 2: 0x1000cd37b node::Abort() [/node/out/Debug/node]
 3: 0x1000cd69f node::OnFatalError(char const*, char const*)
    [/node/out/Debug/node]
 4: 0x1004df0b1 v8::Utils::ReportApiFailure(char const*, char const*)
    [/nodejs/node/out/Debug/node]
 5: 0x100a8c0a9 v8::internal::HandleScope::Extend(
        v8::internal::Isolate*)
    [/node/out/Debug/node]
 6: 0x1004e4229 v8::EmbedderDataFor(v8::Context*,
                                    int, bool,
                                    char const*)
    [/node/out/Debug/node]
 7: 0x1004e43fa v8::Context::SlowGetAlignedPointerFromEmbedderData(int)
    [/node/out/Debug/node]
 8: 0x10001c26b v8::Context::GetAlignedPointerFromEmbedderData(int)
    [/node/out/Debug/node]
 9: 0x1000144ea node::Environment::GetCurrent(v8::Local<v8::Context>)
    [/node/out/Debug/node]
10: 0x1000f49e2 napi_env__::node_env() const
    [/node/out/Debug/node]
11: 0x1000f9885
    (anonymous namespace)::v8impl::ThreadSafeFunction::
        CloseHandlesAndMaybeDelete(bool)
    [/node/out/Debug/node]
12: 0x1000fb34f (anonymous namespace)::v8impl::ThreadSafeFunction::
        DispatchOne()
    [/node/out/Debug/node]
13: 0x1000fb129
    (anonymous namespace)::v8impl::ThreadSafeFunction::
        IdleCb(uv_idle_s*)
    [/node/out/Debug/node]
14: 0x1011a1b69 uv__run_idle
    [/node/out/Debug/node]
15: 0x101198179 uv_run
    [/node/out/Debug/node]
16: 0x1000dfca1
    node::Start(...)
    [/node/out/Debug/node]
17: 0x1000dae50 node::Start(...)
    [/node/out/Debug/node]
18: 0x1000da56f node::Start(int, char**)
    [/node/out/Debug/node]
19: 0x10141112e main
    [/node/out/Debug/node]
20: 0x100001034 start
    [/node/out/Debug/node]
Abort trap: 6

This commit adds two HandleScope's, one to CloseHandlesAndMaybeDelete
and one to the lambda.

SlowGetAlignedPointerFromEmbedderData will only be called for debug
builds:
https://github.com/v8/v8/blob/2ef0aa662fe907a1b36ac1abe7d77ad2bcd27733
/include/v8.h#L10440-L10447

PR-URL: nodejs#24011
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gireeshpunathil
test: mark test_threadsafe_function/test as flaky
e0e1ac2 
The test fails consistently on windows-fanned with vs2017.
mark it as flaky while the issue is being progressed, and
to keep CI green / amber.

Ref: nodejs#23621
PR-URL: nodejs#24714
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@gabrielschulhof
Copy link
Contributor Author

Re-based. CI: https://ci.nodejs.org/job/node-test-pull-request/19859/

@gabrielschulhof
Copy link
Contributor Author

Resumed as: https://ci.nodejs.org/job/node-test-pull-request/19860/

@MylesBorins MylesBorins dismissed their stale review January 3, 2019 22:13

not blocking for now

@MylesBorins
Copy link
Contributor

Some of these commits are Semver-Minor. I have very mixed feelings around this, but as someone who has used Chokidar and know what a pain it can be I'm open to fixing this.

We have also discussed backporting namespaces if they land in a reasonable amount of time. Perhaps we can couple the two in a semver-minor as well as some crypto changes that I know @tniessen was looking to have backported to 8.x

I've opened an issue to discuss this nodejs/Release#405

I've dismissed my blocking review, but would like us to reach a conclusion in the release issue before landing this.

@addaleax
Copy link
Member

addaleax commented Jan 3, 2019

@gabrielschulhof I think the differences that you pointed out are all very much fine here 👍

@pipobscure
Copy link
Contributor

This is the only thing preventing us from deprecating [email protected] entirely in april.
So PLEASE merge this (I'm begging here). Once this is out and v6.x drops out of the picture, we can reduce [email protected] support to v8.x+ and just have the NAPI version.

So PLEASE 👍 I beg you!

@pipobscure pipobscure mentioned this pull request Jan 15, 2019
Merged
@paulmillr
Copy link

@MylesBorins @gabrielschulhof ping please

MylesBorins
MylesBorins approved these changes Jan 18, 2019
View reviewed changes
Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

MylesBorins pushed a commit that referenced this pull request Jan 18, 2019
@MylesBorins
n-api: add API for asynchronous functions
64f232c 
Bundle a `uv_async_t`, a `uv_idle_t`, a `uv_mutex_t`, a `uv_cond_t`,
and a `v8::Persistent<v8::Function>` to make it possible to call into JS
from another thread. The API accepts a void data pointer and a callback
which will be invoked on the loop thread and which will receive the
`napi_value` representing the JavaScript function to call so as to
perform the call into JS. The callback is run inside a
`node::CallbackScope`.

A `std::queue<void*>` is used to store calls from the secondary
threads, and an idle loop is started by the `uv_async_t` callback on the
loop thread to drain the queue, calling into JS with each item.

Items can be added to the queue blockingly or non-blockingly.

The thread-safe function can be referenced or unreferenced, with the
same semantics as libuv handles.

Re: nodejs/help#1035
Re: #20964
Fixes: #13512
Backport-PR-URL: #25002
PR-URL: #17887
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 18, 2019
@cjihrig @MylesBorins
n-api: fix compiler warning
ca8ba15 
private field 'async_context' is not used [-Wunused-private-field]

PR-URL: #21597
Refs: #17887
Backport-PR-URL: #25002
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 18, 2019
@MylesBorins
n-api: guard against cond null dereference
7b68d89 
A condition variable is only created by the thread-safe function if the
queue size is set to something larger than zero. This adds null-checks
around the condition variable and tests for the case where the queue
size is zero.

Fixes: nodejs/help#1387
PR-URL: #21871
Backport-PR-URL: #25002
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 18, 2019
@mhdawson @MylesBorins
src: fix may be uninitialized warning in n-api
c275534 
Backport-PR-URL: #25002
PR-URL: #21898
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 18, 2019
@ralphtheninja @MylesBorins
n-api: remove idle_running from TsFn
311e1f1 
The idle_running member variable in TsFn is always false and can
therefore be removed.

Backport-PR-URL: #25002
PR-URL: #22520
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 18, 2019
@MylesBorins
n-api: clean up thread-safe function
a0fa101 
* Move class `TsFn` to name space `v8impl` and rename it to
  `ThreadSafeFunction`
* Remove `NAPI_EXTERN` from API declarations, because it's only needed
  in the header file.

Backport-PR-URL: #25002
PR-URL: #22259
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 18, 2019
@ralphtheninja @MylesBorins
doc: fix optional parameters in n-api.md
ffbfe34 
The thread_finalize_data and thread_finalize_cb parameters in
napi_create_threadsafe_function are optional.

Backport-PR-URL: #25002
PR-URL: #22998
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 18, 2019
@danbev @MylesBorins
n-api: add missing handle scopes
cffe85c 
Currently when building with --debug
test/addons-napi/test_threadsafe_function will error:

$  out/Debug/node test/addons-napi/test_threadsafe_function/test.js
FATAL ERROR: v8::HandleScope::CreateHandle()
  Cannot create a handle without a HandleScope
 1: 0x10004e287 node::DumpBacktrace(__sFILE*) [node/out/Debug/node]
 2: 0x1000cd37b node::Abort() [/node/out/Debug/node]
 3: 0x1000cd69f node::OnFatalError(char const*, char const*)
    [/node/out/Debug/node]
 4: 0x1004df0b1 v8::Utils::ReportApiFailure(char const*, char const*)
    [/nodejs/node/out/Debug/node]
 5: 0x100a8c0a9 v8::internal::HandleScope::Extend(
        v8::internal::Isolate*)
    [/node/out/Debug/node]
 6: 0x1004e4229 v8::EmbedderDataFor(v8::Context*,
                                    int, bool,
                                    char const*)
    [/node/out/Debug/node]
 7: 0x1004e43fa v8::Context::SlowGetAlignedPointerFromEmbedderData(int)
    [/node/out/Debug/node]
 8: 0x10001c26b v8::Context::GetAlignedPointerFromEmbedderData(int)
    [/node/out/Debug/node]
 9: 0x1000144ea node::Environment::GetCurrent(v8::Local<v8::Context>)
    [/node/out/Debug/node]
10: 0x1000f49e2 napi_env__::node_env() const
    [/node/out/Debug/node]
11: 0x1000f9885
    (anonymous namespace)::v8impl::ThreadSafeFunction::
        CloseHandlesAndMaybeDelete(bool)
    [/node/out/Debug/node]
12: 0x1000fb34f (anonymous namespace)::v8impl::ThreadSafeFunction::
        DispatchOne()
    [/node/out/Debug/node]
13: 0x1000fb129
    (anonymous namespace)::v8impl::ThreadSafeFunction::
        IdleCb(uv_idle_s*)
    [/node/out/Debug/node]
14: 0x1011a1b69 uv__run_idle
    [/node/out/Debug/node]
15: 0x101198179 uv_run
    [/node/out/Debug/node]
16: 0x1000dfca1
    node::Start(...)
    [/node/out/Debug/node]
17: 0x1000dae50 node::Start(...)
    [/node/out/Debug/node]
18: 0x1000da56f node::Start(int, char**)
    [/node/out/Debug/node]
19: 0x10141112e main
    [/node/out/Debug/node]
20: 0x100001034 start
    [/node/out/Debug/node]
Abort trap: 6

This commit adds two HandleScope's, one to CloseHandlesAndMaybeDelete
and one to the lambda.

SlowGetAlignedPointerFromEmbedderData will only be called for debug
builds:
https://github.com/v8/v8/blob/2ef0aa662fe907a1b36ac1abe7d77ad2bcd27733
/include/v8.h#L10440-L10447

Backport-PR-URL: #25002
PR-URL: #24011
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 18, 2019
@gireeshpunathil @MylesBorins
test: mark test_threadsafe_function/test as flaky
b44bd88 
The test fails consistently on windows-fanned with vs2017.
mark it as flaky while the issue is being progressed, and
to keep CI green / amber.

Ref: #23621
Backport-PR-URL: #25002
PR-URL: #24714
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@MylesBorins
Copy link
Contributor

MylesBorins commented Jan 18, 2019 •
edited
Loading

landed in 48da8be...b44bd88

@paulmillr
Copy link

Huge. Thanks Myles!

@pipobscure
Copy link
Contributor

Awesome 🎉🎉🎉🎉

rvagg pushed a commit that referenced this pull request Feb 28, 2019
@rvagg
n-api: add API for asynchronous functions
759a018 
Bundle a `uv_async_t`, a `uv_idle_t`, a `uv_mutex_t`, a `uv_cond_t`,
and a `v8::Persistent<v8::Function>` to make it possible to call into JS
from another thread. The API accepts a void data pointer and a callback
which will be invoked on the loop thread and which will receive the
`napi_value` representing the JavaScript function to call so as to
perform the call into JS. The callback is run inside a
`node::CallbackScope`.

A `std::queue<void*>` is used to store calls from the secondary
threads, and an idle loop is started by the `uv_async_t` callback on the
loop thread to drain the queue, calling into JS with each item.

Items can be added to the queue blockingly or non-blockingly.

The thread-safe function can be referenced or unreferenced, with the
same semantics as libuv handles.

Re: nodejs/help#1035
Re: #20964
Fixes: #13512
Backport-PR-URL: #25002
PR-URL: #17887
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@cjihrig @rvagg
n-api: fix compiler warning
c5a11dc 
private field 'async_context' is not used [-Wunused-private-field]

PR-URL: #21597
Refs: #17887
Backport-PR-URL: #25002
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@rvagg
n-api: guard against cond null dereference
3675059 
A condition variable is only created by the thread-safe function if the
queue size is set to something larger than zero. This adds null-checks
around the condition variable and tests for the case where the queue
size is zero.

Fixes: nodejs/help#1387
PR-URL: #21871
Backport-PR-URL: #25002
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@mhdawson @rvagg
src: fix may be uninitialized warning in n-api
67b6e0d 
Backport-PR-URL: #25002
PR-URL: #21898
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@ralphtheninja @rvagg
n-api: remove idle_running from TsFn
09b88aa 
The idle_running member variable in TsFn is always false and can
therefore be removed.

Backport-PR-URL: #25002
PR-URL: #22520
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Gus Caplan <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@rvagg
n-api: clean up thread-safe function
902b079 
* Move class `TsFn` to name space `v8impl` and rename it to
  `ThreadSafeFunction`
* Remove `NAPI_EXTERN` from API declarations, because it's only needed
  in the header file.

Backport-PR-URL: #25002
PR-URL: #22259
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@ralphtheninja @rvagg
doc: fix optional parameters in n-api.md
6b7cccc 
The thread_finalize_data and thread_finalize_cb parameters in
napi_create_threadsafe_function are optional.

Backport-PR-URL: #25002
PR-URL: #22998
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@danbev @rvagg
n-api: add missing handle scopes
fe43282 
Currently when building with --debug
test/addons-napi/test_threadsafe_function will error:

$  out/Debug/node test/addons-napi/test_threadsafe_function/test.js
FATAL ERROR: v8::HandleScope::CreateHandle()
  Cannot create a handle without a HandleScope
 1: 0x10004e287 node::DumpBacktrace(__sFILE*) [node/out/Debug/node]
 2: 0x1000cd37b node::Abort() [/node/out/Debug/node]
 3: 0x1000cd69f node::OnFatalError(char const*, char const*)
    [/node/out/Debug/node]
 4: 0x1004df0b1 v8::Utils::ReportApiFailure(char const*, char const*)
    [/nodejs/node/out/Debug/node]
 5: 0x100a8c0a9 v8::internal::HandleScope::Extend(
        v8::internal::Isolate*)
    [/node/out/Debug/node]
 6: 0x1004e4229 v8::EmbedderDataFor(v8::Context*,
                                    int, bool,
                                    char const*)
    [/node/out/Debug/node]
 7: 0x1004e43fa v8::Context::SlowGetAlignedPointerFromEmbedderData(int)
    [/node/out/Debug/node]
 8: 0x10001c26b v8::Context::GetAlignedPointerFromEmbedderData(int)
    [/node/out/Debug/node]
 9: 0x1000144ea node::Environment::GetCurrent(v8::Local<v8::Context>)
    [/node/out/Debug/node]
10: 0x1000f49e2 napi_env__::node_env() const
    [/node/out/Debug/node]
11: 0x1000f9885
    (anonymous namespace)::v8impl::ThreadSafeFunction::
        CloseHandlesAndMaybeDelete(bool)
    [/node/out/Debug/node]
12: 0x1000fb34f (anonymous namespace)::v8impl::ThreadSafeFunction::
        DispatchOne()
    [/node/out/Debug/node]
13: 0x1000fb129
    (anonymous namespace)::v8impl::ThreadSafeFunction::
        IdleCb(uv_idle_s*)
    [/node/out/Debug/node]
14: 0x1011a1b69 uv__run_idle
    [/node/out/Debug/node]
15: 0x101198179 uv_run
    [/node/out/Debug/node]
16: 0x1000dfca1
    node::Start(...)
    [/node/out/Debug/node]
17: 0x1000dae50 node::Start(...)
    [/node/out/Debug/node]
18: 0x1000da56f node::Start(int, char**)
    [/node/out/Debug/node]
19: 0x10141112e main
    [/node/out/Debug/node]
20: 0x100001034 start
    [/node/out/Debug/node]
Abort trap: 6

This commit adds two HandleScope's, one to CloseHandlesAndMaybeDelete
and one to the lambda.

SlowGetAlignedPointerFromEmbedderData will only be called for debug
builds:
https://github.com/v8/v8/blob/2ef0aa662fe907a1b36ac1abe7d77ad2bcd27733
/include/v8.h#L10440-L10447

Backport-PR-URL: #25002
PR-URL: #24011
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@gireeshpunathil @rvagg
test: mark test_threadsafe_function/test as flaky
60db455 
The test fails consistently on windows-fanned with vs2017.
mark it as flaky while the issue is being progressed, and
to keep CI green / amber.

Ref: #23621
Backport-PR-URL: #25002
PR-URL: #24714
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@gabrielschulhof gabrielschulhof deleted the backport-tsfn-to-v8.x branch June 14, 2019 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

@addaleax addaleax Awaiting requested review from addaleax

@bnoordhuis bnoordhuis Awaiting requested review from bnoordhuis

@digitalinfinity digitalinfinity Awaiting requested review from digitalinfinity

Assignees
No one assigned
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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.