GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — buffer,n-api: release external buffers from BackingStore callback · nodejs/node@24faa37

File tree

Expand file treeCollapse file tree

Open diff view settings

Filter options

src
- js_native_api_v8.cc
- node_buffer.cc
test
- addons/null-buffer-neuter
  - binding.cc
  - test.js
- node-api/test_buffer
  - test.js

Expand file treeCollapse file tree

Open diff view settings

Collapse file

`‎src/js_native_api_v8.cc‎`

Copy file name to clipboardExpand all lines: src/js_native_api_v8.cc

+21-64Lines changed: 21 additions & 64 deletions

Original file line number	Diff line number	Diff line change
`@@ -371,39 +371,6 @@ class Reference : public RefBase {`
`371`	`371`	`v8impl::Persistent<v8::Value> _persistent;`
`372`	`372`	`};`
`373`	`373`
`374`		`-class ArrayBufferReference final : public Reference {`
`375`		`- public:`
`376`		`- // Same signatures for ctor and New() as Reference, except this only works`
`377`		`- // with ArrayBuffers:`
`378`		`- template <typename... Args>`
`379`		`- explicit ArrayBufferReference(napi_env env,`
`380`		`- v8::Local<v8::ArrayBuffer> value,`
`381`		`- Args&&... args)`
`382`		`- : Reference(env, value, std::forward<Args>(args)...) {}`
`383`		`-`
`384`		`- template <typename... Args>`
`385`		`- static ArrayBufferReference* New(napi_env env,`
`386`		`- v8::Local<v8::ArrayBuffer> value,`
`387`		`- Args&&... args) {`
`388`		`- return new ArrayBufferReference(env, value, std::forward<Args>(args)...);`
`389`		`- }`
`390`		`-`
`391`		`- private:`
`392`		`- inline void Finalize(bool is_env_teardown) override {`
`393`		`- if (is_env_teardown) {`
`394`		`- v8::HandleScope handle_scope(_env->isolate);`
`395`		`- v8::Local<v8::Value> obj = Get();`
`396`		`- CHECK(!obj.IsEmpty());`
`397`		`- CHECK(obj->IsArrayBuffer());`
`398`		`- v8::Local<v8::ArrayBuffer> ab = obj.As<v8::ArrayBuffer>();`
`399`		`- if (ab->IsDetachable())`
`400`		`- ab->Detach();`
`401`		`- }`
`402`		`-`
`403`		`- Reference::Finalize(is_env_teardown);`
`404`		`- }`
`405`		`-};`
`406`		`-`
`407`	`374`	`enum UnwrapAction {`
`408`	`375`	`KeepWrap,`
`409`	`376`	`RemoveWrap`
`@@ -2710,37 +2677,27 @@ napi_status napi_create_external_arraybuffer(napi_env env,`
`2710`	`2677`	`napi_finalize finalize_cb,`
`2711`	`2678`	`void* finalize_hint,`
`2712`	`2679`	`napi_value* result) {`
`2713`		`- NAPI_PREAMBLE(env);`
`2714`		`- CHECK_ARG(env, result);`
`2715`		`-`
`2716`		`- v8::Isolate* isolate = env->isolate;`
`2717`		`- // The buffer will be freed with v8impl::ArrayBufferReference::New()`
`2718`		`- // below, hence this BackingStore does not need to free the buffer.`
`2719`		`- std::unique_ptr<v8::BackingStore> backing =`
`2720`		`- v8::ArrayBuffer::NewBackingStore(external_data,`
`2721`		`- byte_length,`
`2722`		`- [](void, size_t, void){},`
`2723`		`- nullptr);`
`2724`		`- v8::Local<v8::ArrayBuffer> buffer =`
`2725`		`- v8::ArrayBuffer::New(isolate, std::move(backing));`
`2726`		`- v8::Maybe<bool> marked = env->mark_arraybuffer_as_untransferable(buffer);`
`2727`		`- CHECK_MAYBE_NOTHING(env, marked, napi_generic_failure);`
`2728`		`-`
`2729`		`- if (finalize_cb != nullptr) {`
`2730`		`- // Create a self-deleting weak reference that invokes the finalizer`
`2731`		`- // callback and detaches the ArrayBuffer if it still exists on Environment`
`2732`		`- // teardown.`
`2733`		`- v8impl::ArrayBufferReference::New(env,`
`2734`		`- buffer,`
`2735`		`- 0,`
`2736`		`- true,`
`2737`		`- finalize_cb,`
`2738`		`- external_data,`
`2739`		`- finalize_hint);`
`2740`		`- }`
`2741`		`-`
`2742`		`- *result = v8impl::JsValueFromV8LocalValue(buffer);`
`2743`		`- return GET_RETURN_STATUS(env);`
	`2680`	`+ // The API contract here is that the cleanup function runs on the JS thread,`
	`2681`	`+ // and is able to use napi_env. Implementing that properly is hard, so use the`
	`2682`	+ // `Buffer` variant for easier implementation.
	`2683`	`+ napi_value buffer;`
	`2684`	`+ napi_status status;`
	`2685`	`+ status = napi_create_external_buffer(`
	`2686`	`+ env,`
	`2687`	`+ byte_length,`
	`2688`	`+ external_data,`
	`2689`	`+ finalize_cb,`
	`2690`	`+ finalize_hint,`
	`2691`	`+ &buffer);`
	`2692`	`+ if (status != napi_ok) return status;`
	`2693`	`+ return napi_get_typedarray_info(`
	`2694`	`+ env,`
	`2695`	`+ buffer,`
	`2696`	`+ nullptr,`
	`2697`	`+ nullptr,`
	`2698`	`+ nullptr,`
	`2699`	`+ result,`
	`2700`	`+ nullptr);`
`2744`	`2701`	`}`
`2745`	`2702`
`2746`	`2703`	`napi_status napi_get_arraybuffer_info(napi_env env,`

Collapse file

`‎src/node_buffer.cc‎`

Copy file name to clipboardExpand all lines: src/node_buffer.cc

+75-65Lines changed: 75 additions & 65 deletions

Original file line number	Diff line number	Diff line change
`@@ -69,109 +69,130 @@ using v8::Uint32;`
`69`	`69`	`using v8::Uint32Array;`
`70`	`70`	`using v8::Uint8Array;`
`71`	`71`	`using v8::Value;`
`72`		`-using v8::WeakCallbackInfo;`
`73`	`72`
`74`	`73`	`namespace {`
`75`	`74`
`76`	`75`	`class CallbackInfo {`
`77`	`76`	`public:`
`78`		`- ~CallbackInfo();`
`79`		`-`
`80`		`- static inline void Free(char* data, void* hint);`
`81`		`- static inline CallbackInfo* New(Environment* env,`
`82`		`- Local<ArrayBuffer> object,`
`83`		`- FreeCallback callback,`
`84`		`- char* data,`
`85`		`- void* hint = nullptr);`
	`77`	`+ static inline Local<ArrayBuffer> CreateTrackedArrayBuffer(`
	`78`	`+ Environment* env,`
	`79`	`+ char* data,`
	`80`	`+ size_t length,`
	`81`	`+ FreeCallback callback,`
	`82`	`+ void* hint);`
`86`	`83`
`87`	`84`	`CallbackInfo(const CallbackInfo&) = delete;`
`88`	`85`	`CallbackInfo& operator=(const CallbackInfo&) = delete;`
`89`	`86`
`90`	`87`	`private:`
`91`	`88`	`static void CleanupHook(void* data);`
`92`		`- static void WeakCallback(const WeakCallbackInfo<CallbackInfo>&);`
`93`		`- inline void WeakCallback(Isolate* isolate);`
	`89`	`+ inline void OnBackingStoreFree();`
	`90`	`+ inline void CallAndResetCallback();`
`94`	`91`	`inline CallbackInfo(Environment* env,`
`95`		`- Local<ArrayBuffer> object,`
`96`	`92`	`FreeCallback callback,`
`97`	`93`	`char* data,`
`98`	`94`	`void* hint);`
`99`	`95`	`Global<ArrayBuffer> persistent_;`
`100`		`- FreeCallback const callback_;`
	`96`	`+ Mutex mutex_; // Protects callback_.`
	`97`	`+ FreeCallback callback_;`
`101`	`98`	`char* const data_;`
`102`	`99`	`void* const hint_;`
`103`	`100`	`Environment* const env_;`
`104`	`101`	`};`
`105`	`102`
`106`	`103`
`107`		`-void CallbackInfo::Free(char* data, void*) {`
`108`		`- ::free(data);`
`109`		`-}`
`110`		`-`
	`104`	`+Local<ArrayBuffer> CallbackInfo::CreateTrackedArrayBuffer(`
	`105`	`+ Environment* env,`
	`106`	`+ char* data,`
	`107`	`+ size_t length,`
	`108`	`+ FreeCallback callback,`
	`109`	`+ void* hint) {`
	`110`	`+ CHECK_NOT_NULL(callback);`
	`111`	`+ CHECK_IMPLIES(data == nullptr, length == 0);`
	`112`	`+`
	`113`	`+ CallbackInfo* self = new CallbackInfo(env, callback, data, hint);`
	`114`	`+ std::unique_ptr<BackingStore> bs =`
	`115`	`+ ArrayBuffer::NewBackingStore(data, length, [](void, size_t, void arg) {`
	`116`	`+ static_cast<CallbackInfo*>(arg)->OnBackingStoreFree();`
	`117`	`+ }, self);`
	`118`	`+ Local<ArrayBuffer> ab = ArrayBuffer::New(env->isolate(), std::move(bs));`
	`119`	`+`
	`120`	`+ // V8 simply ignores the BackingStore deleter callback if data == nullptr,`
	`121`	`+ // but our API contract requires it being called.`
	`122`	`+ if (data == nullptr) {`
	`123`	`+ ab->Detach();`
	`124`	+ self->OnBackingStoreFree(); // This calls `callback` asynchronously.
	`125`	`+ } else {`
	`126`	`+ // Store the ArrayBuffer so that we can detach it later.`
	`127`	`+ self->persistent_.Reset(env->isolate(), ab);`
	`128`	`+ self->persistent_.SetWeak();`
	`129`	`+ }`
`111`	`130`
`112`		`-CallbackInfo* CallbackInfo::New(Environment* env,`
`113`		`- Local<ArrayBuffer> object,`
`114`		`- FreeCallback callback,`
`115`		`- char* data,`
`116`		`- void* hint) {`
`117`		`- return new CallbackInfo(env, object, callback, data, hint);`
	`131`	`+ return ab;`
`118`	`132`	`}`
`119`	`133`
`120`	`134`
`121`	`135`	`CallbackInfo::CallbackInfo(Environment* env,`
`122`		`- Local<ArrayBuffer> object,`
`123`	`136`	`FreeCallback callback,`
`124`	`137`	`char* data,`
`125`	`138`	`void* hint)`
`126`		`- : persistent_(env->isolate(), object),`
`127`		`- callback_(callback),`
	`139`	`+ : callback_(callback),`
`128`	`140`	`data_(data),`
`129`	`141`	`hint_(hint),`
`130`	`142`	`env_(env) {`
`131`		`- std::shared_ptr<BackingStore> obj_backing = object->GetBackingStore();`
`132`		`- CHECK_EQ(data_, static_cast<char*>(obj_backing->Data()));`
`133`		`- if (object->ByteLength() != 0)`
`134`		`- CHECK_NOT_NULL(data_);`
`135`		`-`
`136`		`- persistent_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter);`
`137`	`143`	`env->AddCleanupHook(CleanupHook, this);`
`138`	`144`	`env->isolate()->AdjustAmountOfExternalAllocatedMemory(sizeof(*this));`
`139`	`145`	`}`
`140`	`146`
`141`		`-`
`142`		`-CallbackInfo::~CallbackInfo() {`
`143`		`- persistent_.Reset();`
`144`		`- env_->RemoveCleanupHook(CleanupHook, this);`
`145`		`-}`
`146`		`-`
`147`		`-`
`148`	`147`	`void CallbackInfo::CleanupHook(void* data) {`
`149`	`148`	`CallbackInfo* self = static_cast<CallbackInfo*>(data);`
`150`	`149`
`151`	`150`	`{`
`152`	`151`	`HandleScope handle_scope(self->env_->isolate());`
`153`	`152`	`Local<ArrayBuffer> ab = self->persistent_.Get(self->env_->isolate());`
`154`		`- CHECK(!ab.IsEmpty());`
`155`		`- if (ab->IsDetachable())`
	`153`	`+ if (!ab.IsEmpty() && ab->IsDetachable()) {`
`156`	`154`	`ab->Detach();`
	`155`	`+ self->persistent_.Reset();`
	`156`	`+ }`
`157`	`157`	`}`
`158`	`158`
`159`		`- self->WeakCallback(self->env_->isolate());`
	`159`	+ // Call the callback in this case, but don't delete `this` yet because the
	`160`	`+ // BackingStore deleter callback will do so later.`
	`161`	`+ self->CallAndResetCallback();`
`160`	`162`	`}`
`161`	`163`
	`164`	`+void CallbackInfo::CallAndResetCallback() {`
	`165`	`+ FreeCallback callback;`
	`166`	`+ {`
	`167`	`+ Mutex::ScopedLock lock(mutex_);`
	`168`	`+ callback = callback_;`
	`169`	`+ callback_ = nullptr;`
	`170`	`+ }`
	`171`	`+ if (callback != nullptr) {`
	`172`	`+ // Clean up all Environment-related state and run the callback.`
	`173`	`+ env_->RemoveCleanupHook(CleanupHook, this);`
	`174`	`+ int64_t change_in_bytes = -static_cast<int64_t>(sizeof(*this));`
	`175`	`+ env_->isolate()->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);`
`162`	`176`
`163`		`-void CallbackInfo::WeakCallback(`
`164`		`- const WeakCallbackInfo<CallbackInfo>& data) {`
`165`		`- CallbackInfo* self = data.GetParameter();`
`166`		`- self->WeakCallback(data.GetIsolate());`
	`177`	`+ callback(data_, hint_);`
	`178`	`+ }`
`167`	`179`	`}`
`168`	`180`
`169`		`-`
`170`		`-void CallbackInfo::WeakCallback(Isolate* isolate) {`
`171`		`- callback_(data_, hint_);`
`172`		`- int64_t change_in_bytes = -static_cast<int64_t>(sizeof(*this));`
`173`		`- isolate->AdjustAmountOfExternalAllocatedMemory(change_in_bytes);`
`174`		`- delete this;`
	`181`	`+void CallbackInfo::OnBackingStoreFree() {`
	`182`	+ // This method should always release the memory for `this`.
	`183`	`+ std::unique_ptr<CallbackInfo> self { this };`
	`184`	`+ Mutex::ScopedLock lock(mutex_);`
	`185`	`+ // If callback_ == nullptr, that means that the callback has already run from`
	`186`	`+ // the cleanup hook, and there is nothing left to do here besides to clean`
	`187`	+ // up the memory involved. In particular, the underlying `Environment` may
	`188`	`+ // be gone at this point, so don’t attempt to call SetImmediateThreadsafe().`
	`189`	`+ if (callback_ == nullptr) return;`
	`190`	`+`
	`191`	`+ env_->SetImmediateThreadsafe([self = std::move(self)](Environment* env) {`
	`192`	`+ CHECK_EQ(self->env_, env); // Consistency check.`
	`193`	`+`
	`194`	`+ self->CallAndResetCallback();`
	`195`	`+ });`
`175`	`196`	`}`
`176`	`197`
`177`	`198`
`@@ -408,26 +429,15 @@ MaybeLocal<Object> New(Environment* env,`
`408`	`429`	`return Local<Object>();`
`409`	`430`	`}`
`410`	`431`
`411`		`-`
`412`		`- // The buffer will be released by a CallbackInfo::New() below,`
`413`		`- // hence this BackingStore callback is empty.`
`414`		`- std::unique_ptr<BackingStore> backing =`
`415`		`- ArrayBuffer::NewBackingStore(data,`
`416`		`- length,`
`417`		`- [](void, size_t, void){},`
`418`		`- nullptr);`
`419`		`- Local<ArrayBuffer> ab = ArrayBuffer::New(env->isolate(),`
`420`		`- std::move(backing));`
	`432`	`+ Local<ArrayBuffer> ab =`
	`433`	`+ CallbackInfo::CreateTrackedArrayBuffer(env, data, length, callback, hint);`
`421`	`434`	`if (ab->SetPrivate(env->context(),`
`422`	`435`	`env->arraybuffer_untransferable_private_symbol(),`
`423`	`436`	`True(env->isolate())).IsNothing()) {`
`424`		`- callback(data, hint);`
`425`	`437`	`return Local<Object>();`
`426`	`438`	`}`
`427`	`439`	`MaybeLocal<Uint8Array> ui = Buffer::New(env, ab, 0, length);`
`428`	`440`
`429`		`- CallbackInfo::New(env, ab, callback, data, hint);`
`430`		`-`
`431`	`441`	`if (ui.IsEmpty())`
`432`	`442`	`return MaybeLocal<Object>();`
`433`	`443`

Collapse file

`‎test/addons/null-buffer-neuter/binding.cc‎`

Copy file name to clipboardExpand all lines: test/addons/null-buffer-neuter/binding.cc

+5-5Lines changed: 5 additions & 5 deletions

Original file line number	Diff line number	Diff line change
`@@ -11,6 +11,10 @@ static void FreeCallback(char* data, void* hint) {`
`11`	`11`	`alive--;`
`12`	`12`	`}`
`13`	`13`
	`14`	`+void IsAlive(const v8::FunctionCallbackInfo<v8::Value>& args) {`
	`15`	`+ args.GetReturnValue().Set(alive);`
	`16`	`+}`
	`17`	`+`
`14`	`18`	`void Run(const v8::FunctionCallbackInfo<v8::Value>& args) {`
`15`	`19`	`v8::Isolate* isolate = args.GetIsolate();`
`16`	`20`	`alive++;`
`@@ -27,15 +31,11 @@ void Run(const v8::FunctionCallbackInfo<v8::Value>& args) {`
`27`	`31`	`char* data = node::Buffer::Data(buf);`
`28`	`32`	`assert(data == nullptr);`
`29`	`33`	`}`
`30`		`-`
`31`		`- isolate->RequestGarbageCollectionForTesting(`
`32`		`- v8::Isolate::kFullGarbageCollection);`
`33`		`-`
`34`		`- assert(alive == 0);`
`35`	`34`	`}`
`36`	`35`
`37`	`36`	`void init(v8::Local<v8::Object> exports) {`
`38`	`37`	`NODE_SET_METHOD(exports, "run", Run);`
	`38`	`+ NODE_SET_METHOD(exports, "isAlive", IsAlive);`
`39`	`39`	`}`
`40`	`40`
`41`	`41`	`NODE_MODULE(NODE_GYP_MODULE_NAME, init)`

Collapse file

`‎test/addons/null-buffer-neuter/test.js‎`

Copy file name to clipboard

+5-1Lines changed: 5 additions & 1 deletion

Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,11 @@`
`1`	`1`	`'use strict';`
`2`	`2`	`// Flags: --expose-gc`
`3`		`-`
`4`	`3`	`const common = require('../../common');`
	`4`	`+const assert = require('assert');`
`5`	`5`	const binding = require(`./build/${common.buildType}/binding`);
`6`	`6`
`7`	`7`	`binding.run();`
	`8`	`+global.gc();`
	`9`	`+setImmediate(() => {`
	`10`	`+ assert.strictEqual(binding.isAlive(), 0);`
	`11`	`+});`

Collapse file

`‎test/node-api/test_buffer/test.js‎`

Copy file name to clipboard

+4-4Lines changed: 4 additions & 4 deletions

Original file line number	Diff line number	Diff line change
`@@ -1,18 +1,18 @@`
`1`	`1`	`'use strict';`
`2`		`-// Flags: --expose-gc`
	`2`	`+// Flags: --expose-gc --no-concurrent-array-buffer-freeing --no-concurrent-array-buffer-sweeping`
`3`	`3`
`4`	`4`	`const common = require('../../common');`
`5`	`5`	const binding = require(`./build/${common.buildType}/test_buffer`);
`6`	`6`	`const assert = require('assert');`
`7`		`-const setImmediatePromise = require('util').promisify(setImmediate);`
	`7`	`+const tick = require('util').promisify(require('../../common/tick'));`
`8`	`8`
`9`	`9`	`(async function() {`
`10`	`10`	`assert.strictEqual(binding.newBuffer().toString(), binding.theText);`
`11`	`11`	`assert.strictEqual(binding.newExternalBuffer().toString(), binding.theText);`
`12`	`12`	`console.log('gc1');`
`13`	`13`	`global.gc();`
`14`	`14`	`assert.strictEqual(binding.getDeleterCallCount(), 0);`
`15`		`- await setImmediatePromise();`
	`15`	`+ await tick(10);`
`16`	`16`	`assert.strictEqual(binding.getDeleterCallCount(), 1);`
`17`	`17`	`assert.strictEqual(binding.copyBuffer().toString(), binding.theText);`
`18`	`18`
`@@ -22,7 +22,7 @@ const setImmediatePromise = require('util').promisify(setImmediate);`
`22`	`22`	`buffer = null;`
`23`	`23`	`global.gc();`
`24`	`24`	`assert.strictEqual(binding.getDeleterCallCount(), 1);`
`25`		`- await setImmediatePromise();`
	`25`	`+ await tick(10);`
`26`	`26`	`console.log('gc2');`
`27`	`27`	`assert.strictEqual(binding.getDeleterCallCount(), 2);`
`28`	`28`	`})().then(common.mustCall());`

0 commit comments

Comments

0 (0)

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.