From d89dff2d630941164e71111a2dbbf98c2ab969fe Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 18 Dec 2019 15:16:02 +0100 Subject: [PATCH 1/4] src,test: use v8::Global instead of v8::Persistent This is in preparation for a lint rule forbidding usage of `v8::Persistent`. --- src/api/async_resource.cc | 1 - src/node.h | 2 +- test/addons/async-hello-world/binding.cc | 3 +-- test/addons/callback-scope/binding.cc | 2 +- 4 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/api/async_resource.cc b/src/api/async_resource.cc index eef80c85f44f4c..0a2437fe6eda5c 100644 --- a/src/api/async_resource.cc +++ b/src/api/async_resource.cc @@ -24,7 +24,6 @@ AsyncResource::AsyncResource(Isolate* isolate, AsyncResource::~AsyncResource() { EmitAsyncDestroy(env_, async_context_); - resource_.Reset(); } MaybeLocal AsyncResource::MakeCallback(Local callback, diff --git a/src/node.h b/src/node.h index 50f85ae338914e..d43693c03389ef 100644 --- a/src/node.h +++ b/src/node.h @@ -893,7 +893,7 @@ class NODE_EXTERN AsyncResource { private: Environment* env_; - v8::Persistent resource_; + v8::Global resource_; async_context async_context_; }; diff --git a/test/addons/async-hello-world/binding.cc b/test/addons/async-hello-world/binding.cc index ff899628c4d56d..2f77aee52ab2d9 100644 --- a/test/addons/async-hello-world/binding.cc +++ b/test/addons/async-hello-world/binding.cc @@ -14,7 +14,7 @@ struct async_req { int input; int output; v8::Isolate* isolate; - v8::Persistent callback; + v8::Global callback; node::async_context context; }; @@ -61,7 +61,6 @@ void AfterAsync(uv_work_t* r) { v8::SealHandleScope seal_handle_scope(isolate); // cleanup node::EmitAsyncDestroy(isolate, req->context); - req->callback.Reset(); delete req; if (try_catch.HasCaught()) { diff --git a/test/addons/callback-scope/binding.cc b/test/addons/callback-scope/binding.cc index 94d5ec91d7f3a2..255d2649b6ea2f 100644 --- a/test/addons/callback-scope/binding.cc +++ b/test/addons/callback-scope/binding.cc @@ -31,7 +31,7 @@ void RunInCallbackScope(const v8::FunctionCallbackInfo& args) { args.GetReturnValue().Set(ret.ToLocalChecked()); } -static v8::Persistent persistent; +static v8::Global persistent; static void Callback(uv_work_t* req, int ignored) { v8::Isolate* isolate = v8::Isolate::GetCurrent(); From f76f6a80a8f7291f5bb6acbe5bc538728425dad4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 18 Dec 2019 16:03:02 +0100 Subject: [PATCH 2/4] fixup! src,test: use v8::Global instead of v8::Persistent --- test/addons/callback-scope/binding.cc | 32 +++++++++++++-------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/test/addons/callback-scope/binding.cc b/test/addons/callback-scope/binding.cc index 255d2649b6ea2f..6bbf5fed35face 100644 --- a/test/addons/callback-scope/binding.cc +++ b/test/addons/callback-scope/binding.cc @@ -4,6 +4,7 @@ #include #include +#include namespace { @@ -31,16 +32,14 @@ void RunInCallbackScope(const v8::FunctionCallbackInfo& args) { args.GetReturnValue().Set(ret.ToLocalChecked()); } -static v8::Global persistent; - static void Callback(uv_work_t* req, int ignored) { v8::Isolate* isolate = v8::Isolate::GetCurrent(); v8::HandleScope scope(isolate); node::CallbackScope callback_scope(isolate, v8::Object::New(isolate), node::async_context{0, 0}); - - v8::Local local = - v8::Local::New(isolate, persistent); + std::unique_ptr> persistent { + static_cast*>(req->data) }; + v8::Local local = persistent->Get(isolate); local->Resolve(isolate->GetCurrentContext(), v8::Undefined(isolate)).ToChecked(); delete req; @@ -49,20 +48,21 @@ static void Callback(uv_work_t* req, int ignored) { static void TestResolveAsync(const v8::FunctionCallbackInfo& args) { v8::Isolate* isolate = args.GetIsolate(); - if (persistent.IsEmpty()) { - persistent.Reset(isolate, v8::Promise::Resolver::New( - isolate->GetCurrentContext()).ToLocalChecked()); + v8::Global* persistent = + new v8::Global( + isolate, + v8::Promise::Resolver::New( + isolate->GetCurrentContext()).ToLocalChecked()); - uv_work_t* req = new uv_work_t; + uv_work_t* req = new uv_work_t; + req->data = static_cast(persistent); - uv_queue_work(node::GetCurrentEventLoop(isolate), - req, - [](uv_work_t*) {}, - Callback); - } + uv_queue_work(node::GetCurrentEventLoop(isolate), + req, + [](uv_work_t*) {}, + Callback); - v8::Local local = - v8::Local::New(isolate, persistent); + v8::Local local = persistent->Get(isolate); args.GetReturnValue().Set(local->GetPromise()); } From 335d4502dc7cb998138d36e4240218a23d3e2a61 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 18 Dec 2019 16:47:38 +0100 Subject: [PATCH 3/4] doc: avoid using v8::Persistent in addon docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use `v8::Global` where possible. For examples where it applies, also clean up the code and make the code multi-threading-ready, for those where that isn’t easily possible, add a warning about that. --- doc/api/addons.md | 63 ++++++++++++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 26 deletions(-) diff --git a/doc/api/addons.md b/doc/api/addons.md index 8b17d6b417b8e8..3b33b9cffe5094 100644 --- a/doc/api/addons.md +++ b/doc/api/addons.md @@ -157,11 +157,11 @@ The context-aware addon can be structured to avoid global static data by performing the following steps: * defining a class which will hold per-addon-instance data. Such -a class should include a `v8::Persistent` which will hold a weak +a class should include a `v8::Global` which will hold a weak reference to the addon's `exports` object. The callback associated with the weak reference will then destroy the instance of the class. * constructing an instance of this class in the addon initializer such that the -`v8::Persistent` is set to the `exports` object. +`v8::Global` is set to the `exports` object. * storing the instance of the class in a `v8::External`, and * passing the `v8::External` to all methods exposed to JavaScript by passing it to the `v8::FunctionTemplate` constructor which creates the native-backed @@ -188,14 +188,6 @@ class AddonData { exports_.SetWeak(this, DeleteMe, WeakCallbackType::kParameter); } - ~AddonData() { - if (!exports_.IsEmpty()) { - // Reset the reference to avoid leaking data. - exports_.ClearWeak(); - exports_.Reset(); - } - } - // Per-addon data. int call_count; @@ -207,7 +199,7 @@ class AddonData { // Weak handle to the "exports" object. An instance of this class will be // destroyed along with the exports object to which it is weakly bound. - v8::Persistent exports_; + v8::Global exports_; }; static void Method(const v8::FunctionCallbackInfo& info) { @@ -830,7 +822,7 @@ class MyObject : public node::ObjectWrap { static void New(const v8::FunctionCallbackInfo& args); static void PlusOne(const v8::FunctionCallbackInfo& args); - static v8::Persistent constructor; + double value_; }; @@ -858,12 +850,10 @@ using v8::Local; using v8::NewStringType; using v8::Number; using v8::Object; -using v8::Persistent; +using v8::ObjectTemplate; using v8::String; using v8::Value; -Persistent MyObject::constructor; - MyObject::MyObject(double value) : value_(value) { } @@ -872,9 +862,15 @@ MyObject::~MyObject() { void MyObject::Init(Local exports) { Isolate* isolate = exports->GetIsolate(); + Local context = isolate->GetCurrentContext(); + + Local addon_data_tpl = ObjectTemplate::New(isolate); + addon_data_tpl->SetInternalFieldCount(1); // 1 field for the MyObject::New() + Local addon_data = + addon_data_tpl->NewInstance(context).ToLocalChecked(); // Prepare constructor template - Local tpl = FunctionTemplate::New(isolate, New); + Local tpl = FunctionTemplate::New(isolate, New, addon_data); tpl->SetClassName(String::NewFromUtf8( isolate, "MyObject", NewStringType::kNormal).ToLocalChecked()); tpl->InstanceTemplate()->SetInternalFieldCount(1); @@ -882,11 +878,11 @@ void MyObject::Init(Local exports) { // Prototype NODE_SET_PROTOTYPE_METHOD(tpl, "plusOne", PlusOne); - Local context = isolate->GetCurrentContext(); - constructor.Reset(isolate, tpl->GetFunction(context).ToLocalChecked()); + Local constructor = tpl->GetFunction(context).ToLocalChecked(); + addon_data->SetInternalField(0, constructor); exports->Set(context, String::NewFromUtf8( isolate, "MyObject", NewStringType::kNormal).ToLocalChecked(), - tpl->GetFunction(context).ToLocalChecked()).FromJust(); + constructor).FromJust(); } void MyObject::New(const FunctionCallbackInfo& args) { @@ -904,7 +900,8 @@ void MyObject::New(const FunctionCallbackInfo& args) { // Invoked as plain function `MyObject(...)`, turn into construct call. const int argc = 1; Local argv[argc] = { args[0] }; - Local cons = Local::New(isolate, constructor); + Local cons = + args.Data().As()->GetInternalField(0).As(); Local result = cons->NewInstance(context, argc, argv).ToLocalChecked(); args.GetReturnValue().Set(result); @@ -1029,7 +1026,7 @@ class MyObject : public node::ObjectWrap { static void New(const v8::FunctionCallbackInfo& args); static void PlusOne(const v8::FunctionCallbackInfo& args); - static v8::Persistent constructor; + static v8::Global constructor; double value_; }; @@ -1047,20 +1044,23 @@ The implementation in `myobject.cc` is similar to the previous example: namespace demo { +using node::AddEnvironmentCleanupHook; using v8::Context; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; +using v8::Global; using v8::Isolate; using v8::Local; using v8::NewStringType; using v8::Number; using v8::Object; -using v8::Persistent; using v8::String; using v8::Value; -Persistent MyObject::constructor; +// Warning! This is not thread-safe, this addon cannot be used for worker +// threads. +Global MyObject::constructor; MyObject::MyObject(double value) : value_(value) { } @@ -1080,6 +1080,10 @@ void MyObject::Init(Isolate* isolate) { Local context = isolate->GetCurrentContext(); constructor.Reset(isolate, tpl->GetFunction(context).ToLocalChecked()); + + AddEnvironmentCleanupHook(isolate, [](void*) { + constructor.Reset(); + }, nullptr); } void MyObject::New(const FunctionCallbackInfo& args) { @@ -1246,7 +1250,7 @@ class MyObject : public node::ObjectWrap { ~MyObject(); static void New(const v8::FunctionCallbackInfo& args); - static v8::Persistent constructor; + static v8::Global constructor; double value_; }; @@ -1264,19 +1268,22 @@ The implementation of `myobject.cc` is similar to before: namespace demo { +using node::AddEnvironmentCleanupHook; using v8::Context; using v8::Function; using v8::FunctionCallbackInfo; using v8::FunctionTemplate; +using v8::Global; using v8::Isolate; using v8::Local; using v8::NewStringType; using v8::Object; -using v8::Persistent; using v8::String; using v8::Value; -Persistent MyObject::constructor; +// Warning! This is not thread-safe, this addon cannot be used for worker +// threads. +Global MyObject::constructor; MyObject::MyObject(double value) : value_(value) { } @@ -1293,6 +1300,10 @@ void MyObject::Init(Isolate* isolate) { Local context = isolate->GetCurrentContext(); constructor.Reset(isolate, tpl->GetFunction(context).ToLocalChecked()); + + AddEnvironmentCleanupHook(isolate, [](void*) { + constructor.Reset(); + }, nullptr); } void MyObject::New(const FunctionCallbackInfo& args) { From 0c0d530024453002ac87cf35e616e712c0d79239 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 18 Dec 2019 15:28:24 +0100 Subject: [PATCH 4/4] tools,src: forbid usage of v8::Persistent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `v8::Persistent` comes with the surprising catch that it requires manual cleanup. `v8::Global` doesn’t, making it easier to use, and additionally provides move semantics. New code should always use `v8::Global`. --- src/node_object_wrap.h | 2 ++ tools/cpplint.py | 26 ++++++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/src/node_object_wrap.h b/src/node_object_wrap.h index 9dce684892a2d8..cb13d84388bcc6 100644 --- a/src/node_object_wrap.h +++ b/src/node_object_wrap.h @@ -65,6 +65,7 @@ class ObjectWrap { } + // NOLINTNEXTLINE(runtime/v8_persistent) inline v8::Persistent& persistent() { return handle_; } @@ -122,6 +123,7 @@ class ObjectWrap { delete wrap; } + // NOLINTNEXTLINE(runtime/v8_persistent) v8::Persistent handle_; }; diff --git a/tools/cpplint.py b/tools/cpplint.py index 40564789549aba..4ea91c0dc04a54 100755 --- a/tools/cpplint.py +++ b/tools/cpplint.py @@ -321,6 +321,7 @@ 'runtime/string', 'runtime/threadsafe_fn', 'runtime/vlog', + 'runtime/v8_persistent', 'whitespace/blank_line', 'whitespace/braces', 'whitespace/comma', @@ -627,6 +628,8 @@ _NULL_TOKEN_PATTERN = re.compile(r'\bNULL\b') +_V8_PERSISTENT_PATTERN = re.compile(r'\bv8::Persistent\b') + _RIGHT_LEANING_POINTER_PATTERN = re.compile(r'[^=|(,\s><);&?:}]' r'(?= 0 or line.find('*/') >= 0: + return + + for match in _V8_PERSISTENT_PATTERN.finditer(line): + error(filename, linenum, 'runtime/v8_persistent', 2, + 'Use v8::Global instead of v8::Persistent') + def CheckLeftLeaningPointer(filename, clean_lines, linenum, error): """Check for left-leaning pointer placement. @@ -4723,6 +4748,7 @@ def CheckStyle(filename, clean_lines, linenum, file_extension, nesting_state, CheckCheck(filename, clean_lines, linenum, error) CheckAltTokens(filename, clean_lines, linenum, error) CheckNullTokens(filename, clean_lines, linenum, error) + CheckV8PersistentTokens(filename, clean_lines, linenum, error) CheckLeftLeaningPointer(filename, clean_lines, linenum, error) classinfo = nesting_state.InnermostClass() if classinfo: