FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: mark ArrayBuffers with free callbacks as untransferable · nodejs/node@9d60499 · GitHub

/ node Public

Commit 9d60499

Browse files
addaleax
authored and
BethGriggs
committed
src: mark ArrayBuffers with free callbacks as untransferable
More precisely, make them untransferable if they were created through *our* APIs, because those do not follow the improved free callback mechanism that V8 uses now. All other ArrayBuffers can be transferred between threads now, the assumption being that they were created in a clean way that follows the V8 API on this. This addresses a TODO comment. Refs: #30339 (comment) PR-URL: #30475 Backport-PR-URL: #31355 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com>
1 parent 367484d commit 9d60499

9 files changed

Lines changed: 86 additions & 4 deletions

File tree

‎src/env.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -151,6 +151,7 @@ constexpr size_t kFsStatsBufferLength =
151151
// "node:" prefix to avoid name clashes with third-party code.
152152
#define PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) \
153153
V(alpn_buffer_private_symbol, "node:alpnBuffer") \
154+
V(arraybuffer_untransferable_private_symbol, "node:untransferableBuffer") \
154155
V(arrow_message_private_symbol, "node:arrowMessage") \
155156
V(contextify_context_private_symbol, "node:contextify:context") \
156157
V(contextify_global_private_symbol, "node:contextify:global") \

‎src/js_native_api_v8.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2614,6 +2614,8 @@ napi_status napi_create_external_arraybuffer(napi_env env,
26142614
v8::Isolate* isolate = env->isolate;
26152615
v8::Local<v8::ArrayBuffer> buffer =
26162616
v8::ArrayBuffer::New(isolate, external_data, byte_length);
2617+
v8::Maybe<bool> marked = env->mark_arraybuffer_as_untransferable(buffer);
2618+
CHECK_MAYBE_NOTHING(env, marked, napi_generic_failure);
26172619

26182620
if (finalize_cb != nullptr) {
26192621
// Create a self-deleting weak reference that invokes the finalizer

‎src/js_native_api_v8.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,10 @@ struct napi_env__ {
3939
inline void Unref() { if ( --refs == 0) delete this; }
4040

4141
virtual bool can_call_into_js() const { return true; }
42+
virtual v8::Maybe<bool> mark_arraybuffer_as_untransferable(
43+
v8::Local<v8::ArrayBuffer> ab) const {
44+
return v8::Just(true);
45+
}
4246

4347
template <typename T, typename U>
4448
void CallIntoModule(T&& call, U&& handle_exception) {

‎src/node_api.cc‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,14 @@ struct node_napi_env__ : public napi_env__ {
2424
bool can_call_into_js() const override {
2525
return node_env()->can_call_into_js();
2626
}
27+
28+
v8::Maybe<bool> mark_arraybuffer_as_untransferable(
29+
v8::Local<v8::ArrayBuffer> ab) const {
30+
return ab->SetPrivate(
31+
context(),
32+
node_env()->arraybuffer_untransferable_private_symbol(),
33+
v8::True(isolate));
34+
}
2735
};
2836

2937
typedef node_napi_env__* node_napi_env;

‎src/node_buffer.cc‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -364,10 +364,8 @@ MaybeLocal<Object> New(Isolate* isolate,
364364
THROW_ERR_BUFFER_CONTEXT_NOT_AVAILABLE(isolate);
365365
return MaybeLocal<Object>();
366366
}
367-
Local<Object> obj;
368-
if (Buffer::New(env, data, length, callback, hint).ToLocal(&obj))
369-
return handle_scope.Escape(obj);
370-
return Local<Object>();
367+
return handle_scope.EscapeMaybe(
368+
Buffer::New(env, data, length, callback, hint));
371369
}
372370

373371

@@ -385,6 +383,12 @@ MaybeLocal<Object> New(Environment* env,
385383
}
386384

387385
Local<ArrayBuffer> ab = ArrayBuffer::New(env->isolate(), data, length);
386+
if (ab->SetPrivate(env->context(),
387+
env->arraybuffer_untransferable_private_symbol(),
388+
True(env->isolate())).IsNothing()) {
389+
callback(data, hint);
390+
return Local<Object>();
391+
}
388392
MaybeLocal<Uint8Array> ui = Buffer::New(env, ab, 0, length);
389393

390394
CallbackInfo::New(env->isolate(), ab, callback, data, hint);

‎src/node_messaging.cc‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -329,6 +329,16 @@ Maybe<bool> Message::Serialize(Environment* env,
329329
!env->isolate_data()->uses_node_allocator()) {
330330
continue;
331331
}
332+
// See https://github.com/nodejs/node/pull/30339#issuecomment-552225353
333+
// for details.
334+
bool untransferrable;
335+
if (!ab->HasPrivate(
336+
context,
337+
env->arraybuffer_untransferable_private_symbol())
338+
.To(&untransferrable)) {
339+
return Nothing<bool>();
340+
}
341+
if (untransferrable) continue;
332342
if (std::find(array_buffers.begin(), array_buffers.end(), ab) !=
333343
array_buffers.end()) {
334344
ThrowDataCloneException(

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
#include <node.h>
2+
#include <node_buffer.h>
3+
#include <v8.h>
4+
5+
using v8::Context;
6+
using v8::Isolate;
7+
using v8::Local;
8+
using v8::Object;
9+
using v8::Value;
10+
11+
char data[] = "hello";
12+
13+
void Initialize(Local<Object> exports,
14+
Local<Value> module,
15+
Local<Context> context) {
16+
Isolate* isolate = context->GetIsolate();
17+
exports->Set(context,
18+
v8::String::NewFromUtf8(
19+
isolate, "buffer", v8::NewStringType::kNormal)
20+
.ToLocalChecked(),
21+
node::Buffer::New(
22+
isolate,
23+
data,
24+
sizeof(data),
25+
[](char* data, void* hint) {},
26+
nullptr).ToLocalChecked()).Check();
27+
}
28+
29+
NODE_MODULE_CONTEXT_AWARE(NODE_GYP_MODULE_NAME, Initialize)

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
'targets': [
3+
{
4+
'target_name': 'binding',
5+
'sources': [ 'binding.cc' ],
6+
'includes': ['../common.gypi'],
7+
}
8+
]
9+
}

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
'use strict';
2+
const common = require('../../common');
3+
const assert = require('assert');
4+
const { MessageChannel } = require('worker_threads');
5+
const { buffer } = require(`./build/${common.buildType}/binding`);
6+
7+
// Test that buffers allocated with a free callback through our APIs are not
8+
// transfered.
9+
10+
const { port1 } = new MessageChannel();
11+
const origByteLength = buffer.byteLength;
12+
port1.postMessage(buffer, [buffer.buffer]);
13+
14+
assert.strictEqual(buffer.byteLength, origByteLength);
15+
assert.notStrictEqual(buffer.byteLength, 0);

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL