Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit e8af569

Browse filesBrowse files
addaleaxaddaleax
committed
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: #30551 Reviewed-By: Gabriel Schulhof <gabriel.schulhof@intel.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
1 parent 40b7621 commit e8af569
Copy full SHA for e8af569

File tree

Expand file treeCollapse file tree

4 files changed

+95
-10
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

4 files changed

+95
-10
lines changed
Open diff view settings
Collapse file

‎src/node_buffer.cc‎

Copy file name to clipboardExpand all lines: src/node_buffer.cc
+36-9Lines changed: 36 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ using v8::Context;
5353
using v8::EscapableHandleScope;
5454
using v8::FunctionCallbackInfo;
5555
using v8::Global;
56+
using v8::HandleScope;
5657
using v8::Int32;
5758
using v8::Integer;
5859
using v8::Isolate;
@@ -73,8 +74,10 @@ namespace {
7374

7475
class CallbackInfo {
7576
public:
77+
~CallbackInfo();
78+
7679
static inline void Free(char* data, void* hint);
77-
static inline CallbackInfo* New(Isolate* isolate,
80+
static inline CallbackInfo* New(Environment* env,
7881
Local<ArrayBuffer> object,
7982
FreeCallback callback,
8083
char* data,
@@ -84,9 +87,10 @@ class CallbackInfo {
8487
CallbackInfo& operator=(const CallbackInfo&) = delete;
8588

8689
private:
90+
static void CleanupHook(void* data);
8791
static void WeakCallback(const WeakCallbackInfo<CallbackInfo>&);
8892
inline void WeakCallback(Isolate* isolate);
89-
inline CallbackInfo(Isolate* isolate,
93+
inline CallbackInfo(Environment* env,
9094
Local<ArrayBuffer> object,
9195
FreeCallback callback,
9296
char* data,
@@ -95,6 +99,7 @@ class CallbackInfo {
9599
FreeCallback const callback_;
96100
char* const data_;
97101
void* const hint_;
102+
Environment* const env_;
98103
};
99104

100105

@@ -103,31 +108,53 @@ void CallbackInfo::Free(char* data, void*) {
103108
}
104109

105110

106-
CallbackInfo* CallbackInfo::New(Isolate* isolate,
111+
CallbackInfo* CallbackInfo::New(Environment* env,
107112
Local<ArrayBuffer> object,
108113
FreeCallback callback,
109114
char* data,
110115
void* hint) {
111-
return new CallbackInfo(isolate, object, callback, data, hint);
116+
return new CallbackInfo(env, object, callback, data, hint);
112117
}
113118

114119

115-
CallbackInfo::CallbackInfo(Isolate* isolate,
120+
CallbackInfo::CallbackInfo(Environment* env,
116121
Local<ArrayBuffer> object,
117122
FreeCallback callback,
118123
char* data,
119124
void* hint)
120-
: persistent_(isolate, object),
125+
: persistent_(env->isolate(), object),
121126
callback_(callback),
122127
data_(data),
123-
hint_(hint) {
128+
hint_(hint),
129+
env_(env) {
124130
ArrayBuffer::Contents obj_c = object->GetContents();
125131
CHECK_EQ(data_, static_cast<char*>(obj_c.Data()));
126132
if (object->ByteLength() != 0)
127133
CHECK_NOT_NULL(data_);
128134

129135
persistent_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);
130-
isolate->AdjustAmountOfExternalAllocatedMemory(sizeof(*this));
136+
env->AddCleanupHook(CleanupHook, this);
137+
env->isolate()->AdjustAmountOfExternalAllocatedMemory(sizeof(*this));
138+
}
139+
140+
141+
CallbackInfo::~CallbackInfo() {
142+
persistent_.Reset();
143+
env_->RemoveCleanupHook(CleanupHook, this);
144+
}
145+
146+
147+
void CallbackInfo::CleanupHook(void* data) {
148+
CallbackInfo* self = static_cast<CallbackInfo*>(data);
149+
150+
{
151+
HandleScope handle_scope(self->env_->isolate());
152+
Local<ArrayBuffer> ab = self->persistent_.Get(self->env_->isolate());
153+
CHECK(!ab.IsEmpty());
154+
ab->Detach();
155+
}
156+
157+
self->WeakCallback(self->env_->isolate());
131158
}
132159

133160

@@ -388,7 +415,7 @@ MaybeLocal<Object> New(Environment* env,
388415
}
389416
MaybeLocal<Uint8Array> ui = Buffer::New(env, ab, 0, length);
390417

391-
CallbackInfo::New(env->isolate(), ab, callback, data, hint);
418+
CallbackInfo::New(env, ab, callback, data, hint);
392419

393420
if (ui.IsEmpty())
394421
return MaybeLocal<Object>();
Collapse file

‎test/addons/worker-buffer-callback/binding.cc‎

Copy file name to clipboardExpand all lines: test/addons/worker-buffer-callback/binding.cc
+10-1Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,24 @@
33
#include <v8.h>
44

55
using v8::Context;
6+
using v8::FunctionCallbackInfo;
67
using v8::Isolate;
78
using v8::Local;
89
using v8::Object;
910
using v8::Value;
1011

12+
uint32_t free_call_count = 0;
1113
char data[] = "hello";
1214

15+
void GetFreeCallCount(const FunctionCallbackInfo<Value>& args) {
16+
args.GetReturnValue().Set(free_call_count);
17+
}
18+
1319
void Initialize(Local<Object> exports,
1420
Local<Value> module,
1521
Local<Context> context) {
1622
Isolate* isolate = context->GetIsolate();
23+
NODE_SET_METHOD(exports, "getFreeCallCount", GetFreeCallCount);
1724
exports->Set(context,
1825
v8::String::NewFromUtf8(
1926
isolate, "buffer", v8::NewStringType::kNormal)
@@ -22,7 +29,9 @@ void Initialize(Local<Object> exports,
2229
isolate,
2330
data,
2431
sizeof(data),
25-
[](char* data, void* hint) {},
32+
[](char* data, void* hint) {
33+
free_call_count++;
34+
},
2635
nullptr).ToLocalChecked()).Check();
2736
}
2837

Collapse file

‎test/addons/worker-buffer-callback/test-free-called.js‎

Copy file name to clipboard
+17Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
'use strict';
2+
const common = require('../../common');
3+
const path = require('path');
4+
const assert = require('assert');
5+
const { Worker } = require('worker_threads');
6+
const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`);
7+
const { getFreeCallCount } = require(binding);
8+
9+
// Test that buffers allocated with a free callback through our APIs are
10+
// released when a Worker owning it exits.
11+
12+
const w = new Worker(`require(${JSON.stringify(binding)})`, { eval: true });
13+
14+
assert.strictEqual(getFreeCallCount(), 0);
15+
w.on('exit', common.mustCall(() => {
16+
assert.strictEqual(getFreeCallCount(), 1);
17+
}));
Collapse file

‎test/cctest/test_environment.cc‎

Copy file name to clipboardExpand all lines: test/cctest/test_environment.cc
+32Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#include "node_buffer.h"
12
#include "node_internals.h"
23
#include "libplatform/libplatform.h"
34

@@ -208,3 +209,34 @@ TEST_F(EnvironmentTest, SetImmediateCleanup) {
208209
EXPECT_EQ(called, 1);
209210
EXPECT_EQ(called_unref, 0);
210211
}
212+
213+
static char hello[] = "hello";
214+
215+
TEST_F(EnvironmentTest, BufferWithFreeCallbackIsDetached) {
216+
// Test that a Buffer allocated with a free callback is detached after
217+
// its callback has been called.
218+
const v8::HandleScope handle_scope(isolate_);
219+
const Argv argv;
220+
221+
int callback_calls = 0;
222+
223+
v8::Local<v8::ArrayBuffer> ab;
224+
{
225+
Env env {handle_scope, argv};
226+
v8::Local<v8::Object> buf_obj = node::Buffer::New(
227+
isolate_,
228+
hello,
229+
sizeof(hello),
230+
[](char* data, void* hint) {
231+
CHECK_EQ(data, hello);
232+
++*static_cast<int*>(hint);
233+
},
234+
&callback_calls).ToLocalChecked();
235+
CHECK(buf_obj->IsUint8Array());
236+
ab = buf_obj.As<v8::Uint8Array>()->Buffer();
237+
CHECK_EQ(ab->ByteLength(), sizeof(hello));
238+
}
239+
240+
CHECK_EQ(callback_calls, 1);
241+
CHECK_EQ(ab->ByteLength(), 0);
242+
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.