A recent change, 215027c8ed, introduced flakiness into our
test suite that exposed an issue with the cleanup hook API design.
Specifically, the signatures of `AddEnvironmentCleanupHook()` and
`RemoveEnvironmentCleanupHook()` are problematic. Both functions
take `Isolate*` arguments, as addons are not generally expected
to have to care about the Node.js `Environment` as a first-class
scope provider.
However, this model made the incorrect assumption that in the
situations in which `RemoveEnvironmentCleanupHook()` would be
invoked an `Environment` would always be associated with the
current `Isolate` (via the current V8 `Context`, if there is one).
This occasionally breaks down when `RemoveEnvironmentCleanupHook()`
is called during garbage collection -- which would be an expected
use case of the functionality, but one that has not been covered
through our tests before 215027c8ed.
Since Node.js guarantees API and ABI stability within a major version,
and this is a bug that is independent from the aforementioned change,
this commit resolves it by adding global mutable state to keep track
off cleanup hooks registered through the Node.js public API.
Obviously, this solution does not represent a desirable long-term
state, and a semver-minor follow up should add an API that does not
require modifications to these data structures, likely based on
the async cleanup hook API which already solves this issue properly.
Refs: https://github.com/nodejs/node/pull/63642
Fixes: https://github.com/nodejs/node/issues/63923
Signed-off-by: Anna Henningsen <anna@addaleax.net>
PR-URL: https://github.com/nodejs/node/pull/63985
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This patch extends `LoadEnvironment` to support loading ES modules,
and adds the following new types:
```cpp
enum class ModuleFormat : uint8_t {
kCommonJS,
kModule,
};
// Data for specifying an entry point script for LoadEnvironment().
// This class uses an opaque layout to allow future additions without
// breaking ABI. Use the setter methods to configure the entry point.
class ModuleData {
void set_source(std::string_view source);
void set_format(ModuleFormat format);
void set_resource_name(std::string_view name);
std::string_view source() const;
ModuleFormat format() const;
std::string_view resource_name() const;
};
class StartExecutionCallbackInfoWithModule {
void set_env(Environment* env);
void set_process_object(v8::Local<v8::Object> process_object);
void set_native_require(v8::Local<v8::Function> native_require);
void set_run_module(v8::Local<v8::Function> run_module);
void set_data(void* data);
Environment* env();
v8::Local<v8::Object> process();
v8::Local<v8::Function> native_require();
v8::Local<v8::Function> run_module();
void* data();
};
```
And two new `LoadEnvironment()` overloads:
```cpp
// Run entry point with ModuleData configuration
MaybeLocal<Value> LoadEnvironment(
Environment* env,
const ModuleData* entry_point,
EmbedderPreloadCallback preload = nullptr);
// Callback-based with new StartExecutionCallbackInfoWithModule
MaybeLocal<Value> LoadEnvironment(
Environment* env,
StartExecutionCallbackWithModule cb,
EmbedderPreloadCallback preload = nullptr,
void* callback_data = nullptr);
```
PR-URL: https://github.com/nodejs/node/pull/61548
Refs: https://github.com/nodejs/node/issues/53565
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Aditi Singh <aditisingh1400@gmail.com>
Instead of polyfilling it with vm.SourceTextModule,
use a built-in source text module loader so that we can
also build the code cache for it at build tiem to
embed the code cache for them in the binary.
Drive-by: instead of inferring how to compile a particular
built-in at run time, do the inferring at build time,
so the function-based built-ins can be compiled using
parameters quickly looked up from a static map, and
the builtins that should be compiled as source text
modules are known internally based on extension in
the source code (at run time, the extensions are all
removed).
PR-URL: https://github.com/nodejs/node/pull/60518
Reviewed-By: Aditi Singh <aditisingh1400@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
This lays the initial groundwork for enabling the
use of IsolateGroups.
Every isolate in V8 is created within a group. When pointer
compression is enabled, all isolates within a single group
are limited to a 4 GB shared pointer cage. By default, all
isolates in the process share the same group, which means
that when running with pointer compression, the entire
process would be limited to a single 4 GB shared pointer
cage. But, we can create as many IsolateGroups as we want,
limited only by the amount of virtual memory available on
the machine.
PR-URL: https://github.com/nodejs/node/pull/60254
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
This is a follow-up to 234c26cca3. The Node-API interface does
not allow us to enforce that values are stored in a specific location,
e.g. on the stack or not; however, V8 requires `Local<>` handles
to be stored on the stack.
To circumvent this restriction, we add the ability to the async handle
stack to store either `Local<>*` pointers or `Global<>*` pointers, with
Node-API making use of the latter.
PR-URL: https://github.com/nodejs/node/pull/59828
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
This is a requirement of the V8 API, but requires a separate
semver-major change (as it is ABI-breaking) to address.
(There's also a similar requirement for `napi_open_callback_scope`
that would not be easily addressable without breaking ABI compatibility
there as well. In real-world situations, it seems extremely unlikely
that the `CallbackScope` would be the only reference to the resource
object.)
PR-URL: https://github.com/nodejs/node/pull/59705
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
This addresses an existing `TODO` to remove the need for a separate
`LocalVector`. Since all relevant handles are already present on the
stack, we can track their addresses instead of storing the objects
in a separate container.
PR-URL: https://github.com/nodejs/node/pull/59704
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
When calling `process.exit()` or on uncaught exceptions as soon as the
process starts, the process will try to terminate immediately. In this
case, there could be a race condition on the unfinished off-thread
system CA loader which tries to access the OpenSSL API which has been
de-inited on the main thread.
PR-URL: https://github.com/nodejs/node/pull/59632
Refs: https://github.com/nodejs/node/pull/59550
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
The order of these calls is important. When the Isolate is disposed,
it may still post tasks to the platform, so it must still be registered
for the task runner to be found from the map. After the isolate is torn
down, we need to remove it from the map before we can free the address,
so that when another Isolate::Allocate() is called, that would not be
allocated to the same address and be registered on an existing map
entry.
PR-URL: https://github.com/nodejs/node/pull/58070
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
As V8 is moving towards built-in CppHeap creation, change the
management so that the automatic CppHeap creation on Node.js's end
is also enforced at Isolate creation time.
1. If embedder uses NewIsolate(), either they use
IsolateSettings::cpp_heap to specify a CppHeap that will be owned
by V8, or if it's not configured, Node.js will create a CppHeap
that will be owned by V8.
2. If the embedder uses SetIsolateUpForNode(),
IsolateSettings::cpp_heap will be ignored (as V8 has deprecated
attaching CppHeap post-isolate-creation). The embedders need to
ensure that the v8::Isolate has a CppHeap attached while it's
still used by Node.js, preferably using v8::CreateParams.
See https://issues.chromium.org/issues/42203693 for details. In
future version of V8, this CppHeap will be created by V8 if not
provided, and we can remove our own "if no CppHeap provided,
create one" code in NewIsolate().
PR-URL: https://github.com/nodejs/node/pull/58070
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Adds a variant of AsyncHooksGetExecutionAsyncId that takes a V8 Context
and returns the async ID belonging to the Environment (if any) of that
Context. Sometimes we want to use Isolate::GetEnteredOrMicrotaskContext
insteads of Isolate::GetCurrentContext (e.g. recording the async ID in
a V8 GC prologue callback) when current context is not set.
PR-URL: https://github.com/nodejs/node/pull/57820
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Previously we have been using the variant of SnapshotCreator that
only passes the external references instead of
v8::Isolate::CreateParams and it's about to be deprecated.
Switch to using the new constructor that takes a fully CreateParams
instead.
This also makes sure that the snapshot building script is using
the Node.js array buffer allocator instead of a separate default
one that was previously used by the old constructor. The zero fill
toggle in the Node.js array buffer allocator would still be ignored
during snapshot building, however, until we fixes the array buffer
allocator and let V8 own the toggle backing store instead, because
otherwise the snapshot would contain the external toggle address
and become unreproducible.
PR-URL: https://github.com/nodejs/node/pull/55337
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Add member to hold the async context frame to AsyncResource to avoid
the need for the async_resource_context_frames_ map in env.
Semver major because it changes ABI.
PR-URL: https://github.com/nodejs/node/pull/56082
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Internal workers are essential to load user scripts and bootstrapped
with internal entrypoints. They should not be waiting for inspectors
even when `--inspect-brk` and `--inspect-wait` were specified, and avoid
blocking main thread to bootstrap.
IsolateData can be created with a specified PerIsolateOptions instead of
creating a copy from the per_process namespace. This also avoids
creating a copy bypassing the parent env's modified options, like
creating a worker thread from a worker thread.
PR-URL: https://github.com/nodejs/node/pull/54219
Fixes: https://github.com/nodejs/node/issues/53681
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Previously V8 would just try to serialize the context data fields
"verbatim" by copying the pointer values. This patch makes
use of the new callbacks so that at serialization, the embedder
data for the context can at least be serialized in a meaningful
way (which are all reset to empty for now). Otherwise the
upstream may have difficulties serializing these pointer values
"verbatim" especially with the introduction of external pointer
tables, even though the verbatim pointer values from a previous
process is meaningless to Node.js.
For Node.js the callback currently just checks that the slots are
know. We will reassign the pointers with newly created native
structures during deserialization and there isn't much we can
reuse for now.
PR-URL: https://github.com/nodejs/node/pull/53217
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5512712/comments/cfc2b28d_c921ac80?tab=comments
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
The `const std::vector<char>&` variant only delegates to a method
that converts it to a `std::string_view` anyway, but adding this
capability to the public API as well means that it’s easier to
store snapshot data in embedding applications (because using
`std::vector<>` enforces heap allocation and `std::string_view`
does not).
PR-URL: https://github.com/nodejs/node/pull/52595
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
This patch implements automatic on-disk code caching that can be enabled
via an environment variable NODE_COMPILE_CACHE.
When set, whenever Node.js compiles a CommonJS or a ECMAScript Module,
it will use on-disk [V8 code cache][] persisted in the specified
directory to speed up the compilation. This may slow down the first
load of a module graph, but subsequent loads of the same module graph
may get a significant speedup if the contents of the modules do not
change. Locally, this speeds up loading of
test/fixtures/snapshot/typescript.js from ~130ms to ~80ms.
To clean up the generated code cache, simply remove the directory.
It will be recreated the next time the same directory is used for
`NODE_COMPILE_CACHE`.
Compilation cache generated by one version of Node.js may not be used
by a different version of Node.js. Cache generated by different versions
of Node.js will be stored separately if the same directory is used
to persist the cache, so they can co-exist.
Caveat: currently when using this with V8 JavaScript code coverage, the
coverage being collected by V8 may be less precise in functions that are
deserialized from the code cache. It's recommended to turn this off when
running tests to generate precise coverage.
Implementation details:
There is one cache file per module on disk. The directory layout
is:
- Compile cache directory (from NODE_COMPILE_CACHE)
- 8b23c8fe: CRC32 hash of CachedDataVersionTag + NODE_VERESION
- 2ea3424d:
- 10860e5a: CRC32 hash of filename + module type
- 431e9adc: ...
- ...
Inside the cache file, there is a header followed by the actual
cache content:
```
[uint32_t] code size
[uint32_t] code hash
[uint32_t] cache size
[uint32_t] cache hash
... compile cache content ...
```
When reading the cache file, we'll also check if the code size
and code hash match the code that the module loader is loading
and whether the cache size and cache hash match the file content
read. If they don't match, or if V8 rejects the cache passed,
we'll ignore the mismatch cache, and regenerate the cache after
compilation succeeds and rewrite it to disk.
PR-URL: https://github.com/nodejs/node/pull/52535
Refs: https://github.com/nodejs/node/issues/47472
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
Anna Henningsen