Comparing changes

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

Navigation Menu

Repository navigation

Comparing changes

Open a pull request

Commits on Apr 1, 2020

This comparison is taking too long to generate.

Uh oh!