Skip to content

Commit

Permalink
Revert napi_create_external_buffer introduction
Browse files Browse the repository at this point in the history
This commit effectively reverts:
nodejs#32463

That change introduces an upward JS API dependency
on the Node API. Since that change fixed a bug, this
will need to be revisited to determine the best way to
fix the original bug without circular dependencies.
  • Loading branch information
sparist committed Feb 3, 2023
1 parent cbe065a commit 182eb70
Showing 1 changed file with 69 additions and 0 deletions.
69 changes: 69 additions & 0 deletions src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,41 @@ inline napi_status ConcludeDeferred(napi_env env,
return GET_RETURN_STATUS(env);
}

#if 1
class ArrayBufferReference final : public Reference {
public:
// Same signatures for ctor and New() as Reference, except this only works
// with ArrayBuffers:
template <typename... Args>
explicit ArrayBufferReference(napi_env env,
v8::Local<v8::ArrayBuffer> value,
Args&&... args)
: Reference(env, value, std::forward<Args>(args)...) {}

template <typename... Args>
static ArrayBufferReference* New(napi_env env,
v8::Local<v8::ArrayBuffer> value,
Args&&... args) {
return new ArrayBufferReference(env, value, std::forward<Args>(args)...);
}

private:
inline void Finalize(bool is_env_teardown) override {
if (is_env_teardown) {
v8::HandleScope handle_scope(_env->isolate);
v8::Local<v8::Value> obj = Get();
CHECK(!obj.IsEmpty());
CHECK(obj->IsArrayBuffer());
v8::Local<v8::ArrayBuffer> ab = obj.As<v8::ArrayBuffer>();
if (ab->IsDetachable())
ab->Detach();
}

Reference::Finalize(is_env_teardown);
}
};
#endif

enum UnwrapAction { KeepWrap, RemoveWrap };

inline napi_status Unwrap(napi_env env,
Expand Down Expand Up @@ -2794,6 +2829,39 @@ napi_create_external_arraybuffer(napi_env env,
napi_finalize finalize_cb,
void* finalize_hint,
napi_value* result) {
#if 1
NAPI_PREAMBLE(env);
CHECK_ARG(env, result);

v8::Isolate* isolate = env->isolate;
// The buffer will be freed with v8impl::ArrayBufferReference::New()
// below, hence this BackingStore does not need to free the buffer.
std::unique_ptr<v8::BackingStore> backing =
v8::ArrayBuffer::NewBackingStore(external_data,
byte_length,
[](void*, size_t, void*){},
nullptr);
v8::Local<v8::ArrayBuffer> buffer =
v8::ArrayBuffer::New(isolate, std::move(backing));
v8::Maybe<bool> marked = env->mark_arraybuffer_as_untransferable(buffer);
CHECK_MAYBE_NOTHING(env, marked, napi_generic_failure);

if (finalize_cb != nullptr) {
// Create a self-deleting weak reference that invokes the finalizer
// callback and detaches the ArrayBuffer if it still exists on Environment
// teardown.
v8impl::ArrayBufferReference::New(env,
buffer,
0,
true,
finalize_cb,
external_data,
finalize_hint);
}

*result = v8impl::JsValueFromV8LocalValue(buffer);
return GET_RETURN_STATUS(env);
#else
// The API contract here is that the cleanup function runs on the JS thread,
// and is able to use napi_env. Implementing that properly is hard, so use the
// `Buffer` variant for easier implementation.
Expand All @@ -2802,6 +2870,7 @@ napi_create_external_arraybuffer(napi_env env,
env, byte_length, external_data, finalize_cb, finalize_hint, &buffer));
return napi_get_typedarray_info(
env, buffer, nullptr, nullptr, nullptr, result, nullptr);
#endif
}

napi_status NAPI_CDECL napi_get_arraybuffer_info(napi_env env,
Expand Down

0 comments on commit 182eb70

Please sign in to comment.