68321eff80A 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 before215027c8ed. 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>
87 lines
2.4 KiB
C++
87 lines
2.4 KiB
C++
#include <node.h>
|
|
#include <v8.h>
|
|
#include <uv.h>
|
|
#include <assert.h>
|
|
#include <stdio.h>
|
|
#include <stdlib.h>
|
|
|
|
using v8::Context;
|
|
using v8::Function;
|
|
using v8::HandleScope;
|
|
using v8::Isolate;
|
|
using v8::Local;
|
|
using v8::MaybeLocal;
|
|
using v8::Object;
|
|
using v8::String;
|
|
using v8::Value;
|
|
|
|
size_t count = 0;
|
|
|
|
struct statically_allocated {
|
|
statically_allocated() {
|
|
assert(count == 0);
|
|
printf("ctor ");
|
|
}
|
|
~statically_allocated() {
|
|
assert(count == 0);
|
|
printf("dtor ");
|
|
}
|
|
} var;
|
|
|
|
void Dummy(void*) {
|
|
assert(0);
|
|
}
|
|
|
|
void Cleanup(void* str) {
|
|
printf("%s ", static_cast<const char*>(str));
|
|
|
|
// Check that calling into JS fails.
|
|
Isolate* isolate = Isolate::GetCurrent();
|
|
HandleScope handle_scope(isolate);
|
|
assert(isolate->InContext());
|
|
Local<Context> context = isolate->GetCurrentContext();
|
|
MaybeLocal<Value> call_result =
|
|
context->Global()->Get(
|
|
context, String::NewFromUtf8Literal(isolate, "Object"))
|
|
.ToLocalChecked().As<Function>()->Call(
|
|
context, v8::Null(isolate), 0, nullptr);
|
|
assert(call_result.IsEmpty());
|
|
}
|
|
|
|
void Initialize(Local<Object> exports,
|
|
Local<Value> module,
|
|
Local<Context> context) {
|
|
Isolate* isolate = Isolate::GetCurrent();
|
|
node::AddEnvironmentCleanupHook(
|
|
isolate, Cleanup, const_cast<void*>(static_cast<const void*>("cleanup")));
|
|
|
|
// Test that adding and removing a cleanup hook works as expected
|
|
{
|
|
node::AddEnvironmentCleanupHook(isolate, Dummy, nullptr);
|
|
node::RemoveEnvironmentCleanupHook(isolate, Dummy, nullptr);
|
|
}
|
|
|
|
// Test that adding and removing a cleanup hook also works if there
|
|
// is no active context during removal
|
|
{
|
|
node::AddEnvironmentCleanupHook(isolate, Dummy, nullptr);
|
|
{
|
|
context->Exit();
|
|
node::RemoveEnvironmentCleanupHook(isolate, Dummy, nullptr);
|
|
context->Enter();
|
|
}
|
|
}
|
|
|
|
if (getenv("addExtraItemToEventLoop") != nullptr) {
|
|
// Add an item to the event loop that we do not clean up in order to make
|
|
// sure that for the main thread, this addon's memory persists even after
|
|
// the Environment instance has been destroyed.
|
|
static uv_async_t extra_async;
|
|
uv_loop_t* loop = node::GetCurrentEventLoop(isolate);
|
|
int err = uv_async_init(loop, &extra_async, [](uv_async_t*) {});
|
|
assert(err == 0);
|
|
uv_unref(reinterpret_cast<uv_handle_t*>(&extra_async));
|
|
}
|
|
}
|
|
|
|
NODE_MODULE_CONTEXT_AWARE(NODE_GYP_MODULE_NAME, Initialize)
|