From b7ef5937444c2693809f149acbb57457d75fe1bb Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 19 Nov 2019 21:34:44 +0100 Subject: [PATCH] buffer: release buffers with free callbacks on env exit Invoke the free callback for a given `Buffer` if it was created with one, and mark the underlying `ArrayBuffer` as detached. This makes sure that the memory is released e.g. when addons inside Workers create such `Buffer`s. PR-URL: https://github.com/nodejs/node/pull/30551 Reviewed-By: Gabriel Schulhof Reviewed-By: Colin Ihrig Reviewed-By: Denys Otrishko Reviewed-By: Michael Dawson --- src/node_buffer.cc | 45 +++++++++++++++---- test/addons/worker-buffer-callback/binding.cc | 11 ++++- .../test-free-called.js | 17 +++++++ test/cctest/test_environment.cc | 32 +++++++++++++ 4 files changed, 95 insertions(+), 10 deletions(-) create mode 100644 test/addons/worker-buffer-callback/test-free-called.js diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 8641270eaecfcd..e5c4655b4ccdb9 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -53,6 +53,7 @@ using v8::Context; using v8::EscapableHandleScope; using v8::FunctionCallbackInfo; using v8::Global; +using v8::HandleScope; using v8::Int32; using v8::Integer; using v8::Isolate; @@ -73,8 +74,10 @@ namespace { class CallbackInfo { public: + ~CallbackInfo(); + static inline void Free(char* data, void* hint); - static inline CallbackInfo* New(Isolate* isolate, + static inline CallbackInfo* New(Environment* env, Local object, FreeCallback callback, char* data, @@ -84,9 +87,10 @@ class CallbackInfo { CallbackInfo& operator=(const CallbackInfo&) = delete; private: + static void CleanupHook(void* data); static void WeakCallback(const WeakCallbackInfo&); inline void WeakCallback(Isolate* isolate); - inline CallbackInfo(Isolate* isolate, + inline CallbackInfo(Environment* env, Local object, FreeCallback callback, char* data, @@ -95,6 +99,7 @@ class CallbackInfo { FreeCallback const callback_; char* const data_; void* const hint_; + Environment* const env_; }; @@ -103,31 +108,53 @@ void CallbackInfo::Free(char* data, void*) { } -CallbackInfo* CallbackInfo::New(Isolate* isolate, +CallbackInfo* CallbackInfo::New(Environment* env, Local object, FreeCallback callback, char* data, void* hint) { - return new CallbackInfo(isolate, object, callback, data, hint); + return new CallbackInfo(env, object, callback, data, hint); } -CallbackInfo::CallbackInfo(Isolate* isolate, +CallbackInfo::CallbackInfo(Environment* env, Local object, FreeCallback callback, char* data, void* hint) - : persistent_(isolate, object), + : persistent_(env->isolate(), object), callback_(callback), data_(data), - hint_(hint) { + hint_(hint), + env_(env) { ArrayBuffer::Contents obj_c = object->GetContents(); CHECK_EQ(data_, static_cast(obj_c.Data())); if (object->ByteLength() != 0) CHECK_NOT_NULL(data_); persistent_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); - isolate->AdjustAmountOfExternalAllocatedMemory(sizeof(*this)); + env->AddCleanupHook(CleanupHook, this); + env->isolate()->AdjustAmountOfExternalAllocatedMemory(sizeof(*this)); +} + + +CallbackInfo::~CallbackInfo() { + persistent_.Reset(); + env_->RemoveCleanupHook(CleanupHook, this); +} + + +void CallbackInfo::CleanupHook(void* data) { + CallbackInfo* self = static_cast(data); + + { + HandleScope handle_scope(self->env_->isolate()); + Local ab = self->persistent_.Get(self->env_->isolate()); + CHECK(!ab.IsEmpty()); + ab->Detach(); + } + + self->WeakCallback(self->env_->isolate()); } @@ -388,7 +415,7 @@ MaybeLocal New(Environment* env, } MaybeLocal ui = Buffer::New(env, ab, 0, length); - CallbackInfo::New(env->isolate(), ab, callback, data, hint); + CallbackInfo::New(env, ab, callback, data, hint); if (ui.IsEmpty()) return MaybeLocal(); diff --git a/test/addons/worker-buffer-callback/binding.cc b/test/addons/worker-buffer-callback/binding.cc index a40876ebb523a6..1141c8a051e077 100644 --- a/test/addons/worker-buffer-callback/binding.cc +++ b/test/addons/worker-buffer-callback/binding.cc @@ -3,17 +3,24 @@ #include using v8::Context; +using v8::FunctionCallbackInfo; using v8::Isolate; using v8::Local; using v8::Object; using v8::Value; +uint32_t free_call_count = 0; char data[] = "hello"; +void GetFreeCallCount(const FunctionCallbackInfo& args) { + args.GetReturnValue().Set(free_call_count); +} + void Initialize(Local exports, Local module, Local context) { Isolate* isolate = context->GetIsolate(); + NODE_SET_METHOD(exports, "getFreeCallCount", GetFreeCallCount); exports->Set(context, v8::String::NewFromUtf8( isolate, "buffer", v8::NewStringType::kNormal) @@ -22,7 +29,9 @@ void Initialize(Local exports, isolate, data, sizeof(data), - [](char* data, void* hint) {}, + [](char* data, void* hint) { + free_call_count++; + }, nullptr).ToLocalChecked()).Check(); } diff --git a/test/addons/worker-buffer-callback/test-free-called.js b/test/addons/worker-buffer-callback/test-free-called.js new file mode 100644 index 00000000000000..2a3cc9e47c22ff --- /dev/null +++ b/test/addons/worker-buffer-callback/test-free-called.js @@ -0,0 +1,17 @@ +'use strict'; +const common = require('../../common'); +const path = require('path'); +const assert = require('assert'); +const { Worker } = require('worker_threads'); +const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`); +const { getFreeCallCount } = require(binding); + +// Test that buffers allocated with a free callback through our APIs are +// released when a Worker owning it exits. + +const w = new Worker(`require(${JSON.stringify(binding)})`, { eval: true }); + +assert.strictEqual(getFreeCallCount(), 0); +w.on('exit', common.mustCall(() => { + assert.strictEqual(getFreeCallCount(), 1); +})); diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc index 0db2963acc9ba3..132f7b44f7db62 100644 --- a/test/cctest/test_environment.cc +++ b/test/cctest/test_environment.cc @@ -1,3 +1,4 @@ +#include "node_buffer.h" #include "node_internals.h" #include "libplatform/libplatform.h" @@ -208,3 +209,34 @@ TEST_F(EnvironmentTest, SetImmediateCleanup) { EXPECT_EQ(called, 1); EXPECT_EQ(called_unref, 0); } + +static char hello[] = "hello"; + +TEST_F(EnvironmentTest, BufferWithFreeCallbackIsDetached) { + // Test that a Buffer allocated with a free callback is detached after + // its callback has been called. + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + + int callback_calls = 0; + + v8::Local ab; + { + Env env {handle_scope, argv}; + v8::Local buf_obj = node::Buffer::New( + isolate_, + hello, + sizeof(hello), + [](char* data, void* hint) { + CHECK_EQ(data, hello); + ++*static_cast(hint); + }, + &callback_calls).ToLocalChecked(); + CHECK(buf_obj->IsUint8Array()); + ab = buf_obj.As()->Buffer(); + CHECK_EQ(ab->ByteLength(), sizeof(hello)); + } + + CHECK_EQ(callback_calls, 1); + CHECK_EQ(ab->ByteLength(), 0); +}