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

WIP still fixing up [v10.x backport] n-api: handle reference delete before finalize #25572

Closed
wants to merge 13 commits into from
Closed

WIP still fixing up [v10.x backport] n-api: handle reference delete before finalize #25572

wants to merge 13 commits into from

Conversation

mhdawson
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Original PR: #24494

psmarshall and others added 12 commits December 26, 2018 11:19
@psmarshall @MylesBorins
deps: cherry-pick 525b396 from V8 upstream
8810051 
Original commit message:

    [cpu-profiler] Fix a leak caused by re-logging existing functions.

    Don't re-log all existing functions during StartProcessorIfNotStarted().
    They will already be in the CodeMap attached to the ProfileGenerator and
    re-logging them causes leaks. See the linked bug for more details.

    Bug: v8:8253
    Change-Id: Ibb1a1ab2431c588e8c3a3a9ff714767cdf61a88e
    Reviewed-on: https://chromium-review.googlesource.com/1256763
    Commit-Queue: Peter Marshall <petermarshall@chromium.org>
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#56336}

Refs: v8/v8@525b396

PR-URL: nodejs#25041
Reviewed-By: Yang Guo <yangguo@chromium.org>
Reviewed-By: Rod Vagg <rod@vagg.org>
@jbunton-atlassian @MylesBorins
doc: update the http.request.setTimeout docs to be accurate
3af7528 
Refs: nodejs#8895

PR-URL: nodejs#25123
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
test: fix test-repl-envvars
48da8be 
In 180f865, the test was changed
so that the `env` argument of `createInternalRepl()` also contained
external environment variables, because keeping them can be necessary
for spawning processes on some systems.

However, this test does not spawn new processes, and relies on the
fact that the environment variables it tests are not already set
(and fails otherwise); therefore, reverting to the original state
should fix this.

Fixes: nodejs#21451
Fixes: nodejs/build#1377
Refs: nodejs#25219

PR-URL: nodejs#25226
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@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: nodejs#20964
Fixes: nodejs#13512
Backport-PR-URL: nodejs#25002
PR-URL: nodejs#17887
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@cjihrig @MylesBorins
n-api: fix compiler warning
ca8ba15 
private field 'async_context' is not used [-Wunused-private-field]

PR-URL: nodejs#21597
Refs: nodejs#17887
Backport-PR-URL: nodejs#25002
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@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: nodejs#21871
Backport-PR-URL: nodejs#25002
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@mhdawson @MylesBorins
src: fix may be uninitialized warning in n-api
c275534 
Backport-PR-URL: nodejs#25002
PR-URL: nodejs#21898
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@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: nodejs#25002
PR-URL: nodejs#22520
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com>
Reviewed-By: Gus Caplan <me@gus.host>
@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: nodejs#25002
PR-URL: nodejs#22259
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@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: nodejs#25002
PR-URL: nodejs#22998
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@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: nodejs#25002
PR-URL: nodejs#24011
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@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: nodejs#23621
Backport-PR-URL: nodejs#25002
PR-URL: nodejs#24714
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Jan 18, 2019
@mhdawson
n-api: handle reference delete before finalize
4f1aeb3 
Crashes were reported during finalization due to
the memory for a reference being deleted and the
finalizer running after the deletion.

This change ensures the deletion of the memory for
the reference only occurs after the finalizer has run.

Fixes: nodejs/node-addon-api#393

Backport-PR-URL: nodejs#25572
PR-URL: nodejs#24494
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@mhdawson mhdawson changed the base branch from master to v8.x January 18, 2019 20:36
@mhdawson mhdawson changed the title [v10.x backport: n-api: handle reference delete before finalize WIP messed up currently [v10.x backport: n-api: handle reference delete before finalize Jan 18, 2019
@mhdawson mhdawson changed the base branch from v8.x to v8.x-staging January 18, 2019 20:38
@mhdawson mhdawson changed the title WIP messed up currently [v10.x backport: n-api: handle reference delete before finalize [v10.x backport: n-api: handle reference delete before finalize Jan 18, 2019
@mhdawson mhdawson changed the title [v10.x backport: n-api: handle reference delete before finalize [v10.x backport] n-api: handle reference delete before finalize Jan 18, 2019
@mhdawson mhdawson changed the title [v10.x backport] n-api: handle reference delete before finalize WIP still fixing up [v10.x backport] n-api: handle reference delete before finalize Jan 18, 2019
@mhdawson mhdawson changed the base branch from v8.x-staging to v10.x-staging January 18, 2019 20:44
@mhdawson mhdawson changed the base branch from v10.x-staging to v8.x January 18, 2019 20:44
@mhdawson
Copy link
Member Author

Did against wrong staging branch. Just going to close and open new PR againts the right one.

@mhdawson mhdawson closed this Jan 18, 2019
@mhdawson mhdawson mentioned this pull request Jan 18, 2019
Closed
3 tasks
@mhdawson mhdawson deleted the backport-24494-to-v8.x branch September 30, 2019 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@mhdawson @nodejs-github-bot @psmarshall @jbunton-atlassian @addaleax @cjihrig @ralphtheninja @danbev @gireeshpunathil