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

[v12.x] backport BaseObjectPtr, SetImmediate(), Worker+report #32301

Closed
wants to merge 20 commits into from
Closed

[v12.x] backport BaseObjectPtr, SetImmediate(), Worker+report #32301

wants to merge 20 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Mar 16, 2020 •
edited
Loading

@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. v12.x labels Mar 16, 2020
@addaleax
Copy link
Member Author

@codebytere Just rebased this, can you take a look?

@addaleax addaleax force-pushed the backport-report-worker-v12.x branch from 633a412 to 30bb555 Compare March 17, 2020 09:02
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@addaleax
Copy link
Member Author

This should not be landed without #32393.

@addaleax addaleax added the blocked PRs that are blocked by other issues or PRs. label Mar 20, 2020
@addaleax addaleax force-pushed the backport-report-worker-v12.x branch from 3207231 to fb98111 Compare March 23, 2020 19:24
@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels Mar 23, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@codebytere codebytere force-pushed the v12.x-staging branch 2 times, most recently from 63a03d2 to d577190 Compare March 31, 2020 23:57
@addaleax @MylesBorins
src: introduce custom smart pointers for BaseObjects
afa5499 
Referring to `BaseObject` instances using standard C++ smart pointers
can interfere with BaseObject’s own cleanup mechanisms
(explicit delete, delete-on-GC and delete-on-cleanup).

Introducing custom smart pointers allows referring to `BaseObject`s
safely while keeping those mechanisms intact.

Refs: nodejs/quic#141
Refs: nodejs/quic#149
Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#30374
Refs: nodejs/quic#165
Reviewed-By: David Carlier <devnexen@gmail.com>
@addaleax @MylesBorins
http2: use custom BaseObject smart pointers
e1b1267 
Refs: nodejs/quic#141
Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#30374
Refs: nodejs/quic#149
Refs: nodejs/quic#165
Reviewed-By: David Carlier <devnexen@gmail.com>
@addaleax @MylesBorins
src: use BaseObjectPtr for keeping channel alive in dns bindings
95bf451 
PR-URL: nodejs#30374
Refs: nodejs/quic#141
Refs: nodejs/quic#149
Refs: nodejs/quic#141
Refs: nodejs/quic#165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@addaleax @MylesBorins
src: remove keep alive option from SetImmediate()
cb196ce 
This is no longer necessary now that the copyable `BaseObjectPtr`
is available (as opposed to the only-movable `v8::Global`).

PR-URL: nodejs#30374
Refs: nodejs/quic#141
Refs: nodejs/quic#149
Refs: nodejs/quic#141
Refs: nodejs/quic#165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
@addaleax @MylesBorins
src: remove HandleWrap instances from list once closed
92cb961 
This allows keeping `BaseObjectPtr`s to `HandleWrap` instances.
Previously, the pointer kept the `HandleWrap` object alive, leaving
the Environment cleanup code that waits for the handle list to drain
in a busy loop, because only the `HandleWrap` destructor removed
the item from the list.

Refs: nodejs/quic#165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

PR-URL: nodejs#30374
Refs: nodejs/quic#141
Refs: nodejs/quic#149
Refs: nodejs/quic#141
Reviewed-By: David Carlier <devnexen@gmail.com>
@addaleax @MylesBorins
src: keep object alive in stream_pipe code
b18531d 
This was overlooked in a489583.

Refs: nodejs#30374

PR-URL: nodejs#30666
Fixes: nodejs#30643
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax @MylesBorins
src: add more can_call_into_js() guards
ece9faf 
This is in preparation for running native `SetImmediate()`
callbacks during shutdown.

PR-URL: nodejs#30666
Fixes: nodejs#30643
Refs: nodejs#30374
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax @MylesBorins
src: no SetImmediate from destructor in stream_pipe code
c3fff25 
Guard against running `SetImmediate()` from the destructor.
The object will not be alive or usable in the callback,
so it does not make sense to attempt to schedule the
`SetImmediate()`.

PR-URL: nodejs#30666
Fixes: nodejs#30643
Refs: nodejs#30374
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax @MylesBorins
src: run native immediates during Environment cleanup
2470035 
This can be necessary, because some parts of the Node.js code base
perform cleanup operations in the Immediate callbacks, e.g. HTTP/2.

This resolves flakiness in an HTTP/2 test that failed when a
`SetImmediate()` callback was not run or destroyed before the
`Environment` destructor started, because that callback held a
strong reference to the `Http2Session` object and the expectation
was that no such objects exist once the `Environment` constructor
starts.

Another, slightly more direct, alternative would have
been to clear the immediate queue rather than to run it. However,
this approach seems to make more sense as code generally assumes
that the `SetImmediate()` callback will always run; For example,
N-API uses an immediate callback to call buffer finalization
callbacks.

Unref’ed immediates are skipped, as the expectation is generally
that they may not run anyway.

Fixes: nodejs#30643

PR-URL: nodejs#30666
Refs: nodejs#30374
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax @MylesBorins
src: better encapsulate native immediate list
973f8ef 
Refactor for clarity and reusability. Make it more obvious that the
list is a FIFO queue.

PR-URL: nodejs#31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@addaleax @MylesBorins
src: exclude C++ SetImmediate() from count
5e1bcae 
There is no real reason to manage a count manually, given that
checking whether there are C++ callbacks is a single pointer
comparison.

This makes it easier to add other kinds of native C++ callbacks
that are managed in a similar way.

PR-URL: nodejs#31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@addaleax @MylesBorins
src: harden running native SetImmediate()s slightly
4d3275b 
Prevent mistakes like the one fixed by the previous commit
by destroying the callback immediately after it has been called.

PR-URL: nodejs#31468
Refs: nodejs#31386
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
@addaleax @MylesBorins
src: delete BaseObjectWeakPtr data when pointee is gone
d85cb07 
Fix the condition for deleting the underlying data pointed to by
a `BaseObjectWeakPtr`, which erroneously skipped that deletion
when `ptr->get()` was `nullptr`. This fixes a memory leak reported
by some of the tests.

Refs: nodejs#30374 (comment)

PR-URL: nodejs#32393
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

Rebased against staging

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: introduce custom smart pointers for BaseObjects
c60780f 
Referring to `BaseObject` instances using standard C++ smart pointers
can interfere with BaseObject’s own cleanup mechanisms
(explicit delete, delete-on-GC and delete-on-cleanup).

Introducing custom smart pointers allows referring to `BaseObject`s
safely while keeping those mechanisms intact.

Refs: nodejs/quic#141
Refs: nodejs/quic#149
Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: #32301
PR-URL: #30374
Refs: nodejs/quic#165
Reviewed-By: David Carlier <devnexen@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
http2: use custom BaseObject smart pointers
4745ac4 
Refs: nodejs/quic#141
Reviewed-By: James M Snell <jasnell@gmail.com>

Backport-PR-URL: #32301
PR-URL: #30374
Refs: nodejs/quic#149
Refs: nodejs/quic#165
Reviewed-By: David Carlier <devnexen@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: use BaseObjectPtr for keeping channel alive in dns bindings
6db84d3 
Backport-PR-URL: #32301
PR-URL: #30374
Refs: nodejs/quic#141
Refs: nodejs/quic#149
Refs: nodejs/quic#141
Refs: nodejs/quic#165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: remove keep alive option from SetImmediate()
e17d314 
This is no longer necessary now that the copyable `BaseObjectPtr`
is available (as opposed to the only-movable `v8::Global`).

Backport-PR-URL: #32301
PR-URL: #30374
Refs: nodejs/quic#141
Refs: nodejs/quic#149
Refs: nodejs/quic#141
Refs: nodejs/quic#165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: remove HandleWrap instances from list once closed
5f95e69 
This allows keeping `BaseObjectPtr`s to `HandleWrap` instances.
Previously, the pointer kept the `HandleWrap` object alive, leaving
the Environment cleanup code that waits for the handle list to drain
in a busy loop, because only the `HandleWrap` destructor removed
the item from the list.

Refs: nodejs/quic#165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>

Backport-PR-URL: #32301
PR-URL: #30374
Refs: nodejs/quic#141
Refs: nodejs/quic#149
Refs: nodejs/quic#141
Reviewed-By: David Carlier <devnexen@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: keep object alive in stream_pipe code
7647bfe 
This was overlooked in a489583.

Refs: #30374

Backport-PR-URL: #32301
PR-URL: #30666
Fixes: #30643
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: add more can_call_into_js() guards
51230f7 
This is in preparation for running native `SetImmediate()`
callbacks during shutdown.

Backport-PR-URL: #32301
PR-URL: #30666
Fixes: #30643
Refs: #30374
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: no SetImmediate from destructor in stream_pipe code
5b65348 
Guard against running `SetImmediate()` from the destructor.
The object will not be alive or usable in the callback,
so it does not make sense to attempt to schedule the
`SetImmediate()`.

Backport-PR-URL: #32301
PR-URL: #30666
Fixes: #30643
Refs: #30374
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: run native immediates during Environment cleanup
2625244 
This can be necessary, because some parts of the Node.js code base
perform cleanup operations in the Immediate callbacks, e.g. HTTP/2.

This resolves flakiness in an HTTP/2 test that failed when a
`SetImmediate()` callback was not run or destroyed before the
`Environment` destructor started, because that callback held a
strong reference to the `Http2Session` object and the expectation
was that no such objects exist once the `Environment` constructor
starts.

Another, slightly more direct, alternative would have
been to clear the immediate queue rather than to run it. However,
this approach seems to make more sense as code generally assumes
that the `SetImmediate()` callback will always run; For example,
N-API uses an immediate callback to call buffer finalization
callbacks.

Unref’ed immediates are skipped, as the expectation is generally
that they may not run anyway.

Fixes: #30643

Backport-PR-URL: #32301
PR-URL: #30666
Refs: #30374
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: better encapsulate native immediate list
5df9698 
Refactor for clarity and reusability. Make it more obvious that the
list is a FIFO queue.

Backport-PR-URL: #32301
PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: exclude C++ SetImmediate() from count
aafb224 
There is no real reason to manage a count manually, given that
checking whether there are C++ callbacks is a single pointer
comparison.

This makes it easier to add other kinds of native C++ callbacks
that are managed in a similar way.

Backport-PR-URL: #32301
PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: add a threadsafe variant of SetImmediate()
748a530 
Add a variant of `SetImmediate()` that can be called from any thread.
This allows removing the `AsyncRequest` abstraction and replaces it
with a more generic mechanism.

Backport-PR-URL: #32301
PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: remove AsyncRequest
7c2c068 
Remove `AsyncRequest` from the source code, and replace its
usage with threadsafe `SetImmediate()` calls. This has the
advantage of being able to pass in any function, rather than
one that is defined when the `AsyncRequest` is “installed”.

This necessitates two changes:

- The stopping flag (which was only used in one case and ignored
  in the other) is now a direct member of the `Environment` class.
- Workers no longer have their own libuv handles, requiring
  manual management of their libuv ref count.

As a drive-by fix, the `can_call_into_js` variable was turned
into an atomic variable. While there have been no bug reports,
the flag is set from `Stop(env)` calls, which are supposed to
be possible from any thread.

Backport-PR-URL: #32301
PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: add interrupts to Environments/Workers
22bf867 
Allow doing what V8’s `v8::Isolate::RequestInterrupt()` does for V8.
This also works when there is no JS code currently executing.

Backport-PR-URL: #32301
PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: move MemoryInfo() for worker code to .cc files
c2176e1 
This is a) the right thing to do anyway because these functions
can not be inlined by the compiler and b) avoids compilation warnings
in the following commit.

Backport-PR-URL: #32301
PR-URL: #31386
Refs: openjs-foundation/summit#240
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
report: add support for Workers
87cfbb2 
Include a report for each sub-Worker of the current Node.js instance.

This adds a feature that is necessary for eventually making the report
feature stable, as was discussed during the last collaborator summit.

Refs: openjs-foundation/summit#240

Backport-PR-URL: #32301
PR-URL: #31386
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: simplify native immediate queue running
cb16aab 
Make `SetImmediate()` behave more like `process.nextTick()`
(which matches how we use it) by also running tasks that have been
added during previous `SetImmediate()` calls.

Backport-PR-URL: #32301
PR-URL: #31502
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
worker: move JoinThread() back into exit callback
c5de212 
de2c68c moved this call to
the destructor, under the assumption that that would essentially
be equivalent to running it as part of the callback since the
worker would be destroyed along with the callback.

However, the actual code in
`Environment::RunAndClearNativeImmediates()` comes with the subtlety
that testing whether a JS exception has been thrown
happens between the invocation of the callback and its destruction,
leaving a possible exception from `JoinThread()` potentially
unhandled (and unintentionally silenced through the `TryCatch`).

This affected exceptions thrown from the `'exit'` event of the
Worker, and made the `parallel/test-worker-message-type-unknown`
test flaky, as the invalid message was sometimes only received
during the Worker thread’s exit handler.

Fix this by moving the `JoinThread()` call back to where it was
before.

Refs: #31386

Backport-PR-URL: #32301
PR-URL: #31468
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: harden running native SetImmediate()s slightly
56a4509 
Prevent mistakes like the one fixed by the previous commit
by destroying the callback immediately after it has been called.

Backport-PR-URL: #32301
PR-URL: #31468
Refs: #31386
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
@addaleax @MylesBorins
src: delete BaseObjectWeakPtr data when pointee is gone
fbd5be6 
Fix the condition for deleting the underlying data pointed to by
a `BaseObjectWeakPtr`, which erroneously skipped that deletion
when `ptr->get()` was `nullptr`. This fixes a memory leak reported
by some of the tests.

Refs: #30374 (comment)

Backport-PR-URL: #32301
PR-URL: #32393
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

green ci!

landed in d568efc...fbd5be6

@MylesBorins MylesBorins closed this Apr 1, 2020
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
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@addaleax @nodejs-github-bot @MylesBorins