From 28f5d3af07b1d9bbeaaeb6b318fe44335f30fde6 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 28 Sep 2025 07:58:59 -0700 Subject: [PATCH 1/2] src: update locks to use DictionaryTemplate ...and other minor cleanups --- src/env_properties.h | 8 +- src/node_locks.cc | 249 +++++++++++++++++-------------------------- src/util-inl.h | 20 ++++ src/util.h | 3 + 4 files changed, 126 insertions(+), 154 deletions(-) diff --git a/src/env_properties.h b/src/env_properties.h index 3b9f4c7ed27fb1..e7f8aff60ef70f 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -92,7 +92,6 @@ V(change_string, "change") \ V(changes_string, "changes") \ V(chunks_sent_since_last_write_string, "chunksSentSinceLastWrite") \ - V(client_id_string, "clientId") \ V(clone_unsupported_type_str, "Cannot clone object of unsupported type.") \ V(clone_transfer_needed_str, \ "Object that needs transfer was found in message but not listed in " \ @@ -165,6 +164,7 @@ V(errstr_string, "errstr") \ V(events_waiting, "eventsWaiting") \ V(events, "events") \ + V(exclusive_string, "exclusive") \ V(exponent_string, "exponent") \ V(exports_string, "exports") \ V(external_stream_string, "_externalStream") \ @@ -191,7 +191,6 @@ V(h2_string, "h2") \ V(handle_string, "handle") \ V(hash_algorithm_string, "hashAlgorithm") \ - V(held_string, "held") \ V(help_text_string, "helpText") \ V(homedir_string, "homedir") \ V(host_string, "host") \ @@ -244,7 +243,6 @@ V(message_string, "message") \ V(messageerror_string, "messageerror") \ V(mgf1_hash_algorithm_string, "mgf1HashAlgorithm") \ - V(mode_string, "mode") \ V(module_string, "module") \ V(modulus_length_string, "modulusLength") \ V(name_string, "name") \ @@ -285,7 +283,6 @@ V(path_string, "path") \ V(pathname_string, "pathname") \ V(pending_handle_string, "pendingHandle") \ - V(pending_string, "pending") \ V(permission_string, "permission") \ V(phase_string, "phase") \ V(pid_string, "pid") \ @@ -325,6 +322,7 @@ V(servername_string, "servername") \ V(session_id_string, "sessionId") \ V(set_string, "set") \ + V(shared_string, "shared") \ V(shell_string, "shell") \ V(signal_string, "signal") \ V(sink_string, "sink") \ @@ -416,6 +414,8 @@ V(js_transferable_constructor_template, v8::FunctionTemplate) \ V(libuv_stream_wrap_ctor_template, v8::FunctionTemplate) \ V(lock_holder_constructor_template, v8::FunctionTemplate) \ + V(lock_info_template, v8::DictionaryTemplate) \ + V(lock_query_template, v8::DictionaryTemplate) \ V(message_port_constructor_template, v8::FunctionTemplate) \ V(module_wrap_constructor_template, v8::FunctionTemplate) \ V(mx_record_template, v8::DictionaryTemplate) \ diff --git a/src/node_locks.cc b/src/node_locks.cc index 0c2dc7ff0bb2ff..f26f112f3524d2 100644 --- a/src/node_locks.cc +++ b/src/node_locks.cc @@ -13,6 +13,7 @@ namespace node::worker::locks { using node::errors::TryCatchScope; using v8::Array; using v8::Context; +using v8::DictionaryTemplate; using v8::Exception; using v8::Function; using v8::FunctionCallbackInfo; @@ -20,35 +21,25 @@ using v8::FunctionTemplate; using v8::HandleScope; using v8::Isolate; using v8::Local; +using v8::LocalVector; using v8::MaybeLocal; using v8::NewStringType; using v8::Object; using v8::ObjectTemplate; using v8::Promise; +using v8::PropertyAttribute; using v8::String; using v8::Value; -static constexpr const char* kSharedMode = "shared"; -static constexpr const char* kExclusiveMode = "exclusive"; -static constexpr const char* kLockStolenError = "LOCK_STOLEN"; - // Reject two promises and return `false` on failure. static bool RejectBoth(Local ctx, Local first, Local second, Local reason) { - if (first->Reject(ctx, reason).IsNothing()) return false; - if (second->Reject(ctx, reason).IsNothing()) return false; - - return true; + return first->Reject(ctx, reason).IsJust() && + second->Reject(ctx, reason).IsJust(); } -static MaybeLocal CreateLockInfoObject(Isolate* isolate, - Local context, - const std::u16string& name, - Lock::Mode mode, - const std::string& client_id); - Lock::Lock(Environment* env, const std::u16string& name, Mode mode, @@ -80,6 +71,33 @@ LockRequest::LockRequest(Environment* env, callback_.Reset(env_->isolate(), callback); } +Local GetLockInfoTemplate(Environment* env) { + auto tmpl = env->lock_info_template(); + if (tmpl.IsEmpty()) { + static constexpr std::string_view names[] = { + "name", + "mode", + "clientId", + }; + tmpl = DictionaryTemplate::New(env->isolate(), names); + env->set_lock_info_template(tmpl); + } + return tmpl; +} + +static MaybeLocal CreateLockInfoObject(Environment* env, + const auto& request) { + auto tmpl = GetLockInfoTemplate(env); + MaybeLocal values[] = { + ToV8Value(env->context(), request.name()), + request.mode() == Lock::Mode::Exclusive ? env->exclusive_string() + : env->shared_string(), + ToV8Value(env->context(), request.client_id()), + }; + + return NewDictionaryInstance(env->context(), tmpl, values); +} + bool LockManager::IsGrantable(const LockRequest* request) const { // Steal requests bypass all normal granting rules if (request->steal()) return true; @@ -302,11 +320,12 @@ void LockManager::ProcessQueue(Environment* env) { if (!if_available_request->callback() ->Call(context, Undefined(isolate), 1, &null_arg) .ToLocal(&callback_result)) { - if (!RejectBoth(context, - if_available_request->waiting_promise(), - if_available_request->released_promise(), - try_catch_scope.Exception())) - return; + // We don't really need to check the return value here since + // we're returning early in either case. + USE(RejectBoth(context, + if_available_request->waiting_promise(), + if_available_request->released_promise(), + try_catch_scope.Exception())); return; } } @@ -338,34 +357,31 @@ void LockManager::ProcessQueue(Environment* env) { isolate, "Failed to attach promise handlers")); } - RejectBoth(context, - if_available_request->waiting_promise(), - if_available_request->released_promise(), - err_val); + USE(RejectBoth(context, + if_available_request->waiting_promise(), + if_available_request->released_promise(), + err_val)); return; } } // After handlers are attached, resolve waiting_promise with the // promise. - if (if_available_request->waiting_promise() + USE(if_available_request->waiting_promise() ->Resolve(context, p) - .IsNothing()) - return; - + .IsNothing()); return; } // Non-promise callback result: settle both promises right away. if (if_available_request->waiting_promise() ->Resolve(context, callback_result) - .IsNothing()) + .IsNothing()) { return; - if (if_available_request->released_promise() + } + USE(if_available_request->released_promise() ->Resolve(context, callback_result) - .IsNothing()) - return; - + .IsNothing()); return; } @@ -391,13 +407,8 @@ void LockManager::ProcessQueue(Environment* env) { existing_lock->mark_stolen(); envs_to_notify.insert(existing_lock->env()); - // Immediately reject the stolen lock's released_promise - Local error_string; - if (!String::NewFromUtf8(isolate, kLockStolenError) - .ToLocal(&error_string)) { - return; - } - Local error = Exception::Error(error_string); + Local error = + Exception::Error(FIXED_ONE_BYTE_STRING(isolate, "LOCK_STOLEN")); if (existing_lock->released_promise() ->Reject(context, error) @@ -444,12 +455,7 @@ void LockManager::ProcessQueue(Environment* env) { // Create and store the new granted lock Local lock_info; - if (!CreateLockInfoObject(isolate, - context, - grantable_request->name(), - grantable_request->mode(), - grantable_request->client_id()) - .ToLocal(&lock_info)) { + if (!CreateLockInfoObject(env, *grantable_request).ToLocal(&lock_info)) { return; } @@ -461,11 +467,12 @@ void LockManager::ProcessQueue(Environment* env) { if (!grantable_request->callback() ->Call(context, Undefined(isolate), 1, &callback_arg) .ToLocal(&callback_result)) { - if (!RejectBoth(context, - grantable_request->waiting_promise(), - grantable_request->released_promise(), - try_catch_scope.Exception())) - return; + // We don't really need to check the return value here since + // we're returning early in either case. + USE(RejectBoth(context, + grantable_request->waiting_promise(), + grantable_request->released_promise(), + try_catch_scope.Exception())); return; } } @@ -509,10 +516,10 @@ void LockManager::ProcessQueue(Environment* env) { isolate, "Failed to attach promise handlers")); } - RejectBoth(context, - grantable_request->waiting_promise(), - grantable_request->released_promise(), - err_val); + USE(RejectBoth(context, + grantable_request->waiting_promise(), + grantable_request->released_promise(), + err_val)); return; } } @@ -580,7 +587,7 @@ void LockManager::Request(const FunctionCallbackInfo& args) { Local callback = args[5].As(); Lock::Mode lock_mode = - mode_str == kSharedMode ? Lock::Mode::Shared : Lock::Mode::Exclusive; + mode_str == "shared" ? Lock::Mode::Shared : Lock::Mode::Exclusive; Local waiting_promise; Local released_promise; @@ -640,65 +647,57 @@ void LockManager::Query(const FunctionCallbackInfo& args) { // Always set the return value first so Javascript gets a promise args.GetReturnValue().Set(resolver->GetPromise()); - Local result = Object::New(isolate); - Local held_list = Array::New(isolate); - Local pending_list = Array::New(isolate); + LocalVector held_list(isolate); + LocalVector pending_list(isolate); LockManager* manager = GetCurrent(); { Mutex::ScopedLock scoped_lock(manager->mutex_); - uint32_t index = 0; Local lock_info; for (const auto& resource_entry : manager->held_locks_) { for (const auto& held_lock : resource_entry.second) { if (held_lock->env() == env) { - if (!CreateLockInfoObject(isolate, - context, - held_lock->name(), - held_lock->mode(), - held_lock->client_id()) - .ToLocal(&lock_info)) { - THROW_ERR_OPERATION_FAILED(env, - "Failed to create lock info object"); - return; - } - if (held_list->Set(context, index++, lock_info).IsNothing()) { - THROW_ERR_OPERATION_FAILED(env, "Failed to build held locks array"); + if (!CreateLockInfoObject(env, *held_lock).ToLocal(&lock_info)) { + // There should already be a pending exception scheduled. return; } + held_list.push_back(lock_info); } } } - index = 0; for (const auto& pending_request : manager->pending_queue_) { if (pending_request->env() == env) { - if (!CreateLockInfoObject(isolate, - context, - pending_request->name(), - pending_request->mode(), - pending_request->client_id()) - .ToLocal(&lock_info)) { - THROW_ERR_OPERATION_FAILED(env, "Failed to create lock info object"); - return; - } - if (pending_list->Set(context, index++, lock_info).IsNothing()) { - THROW_ERR_OPERATION_FAILED(env, - "Failed to build pending locks array"); + if (!CreateLockInfoObject(env, *pending_request).ToLocal(&lock_info)) { + // There should already be a pending exception scheduled. return; } + pending_list.push_back(lock_info); } } } - if (result->Set(context, env->held_string(), held_list).IsNothing() || - result->Set(context, env->pending_string(), pending_list).IsNothing()) { - THROW_ERR_OPERATION_FAILED(env, "Failed to build query result object"); - return; + auto tmpl = env->lock_query_template(); + if (tmpl.IsEmpty()) { + static constexpr std::string_view names[] = { + "held", + "pending", + }; + tmpl = DictionaryTemplate::New(isolate, names); + env->set_lock_query_template(tmpl); } - if (resolver->Resolve(context, result).IsNothing()) return; + MaybeLocal values[] = { + Array::New(isolate, held_list.data(), held_list.size()), + Array::New(isolate, pending_list.data(), pending_list.size()), + }; + + Local result; + if (NewDictionaryInstance(env->context(), tmpl, values).ToLocal(&result)) { + // There's no reason to check IsNothing here since we're just returning. + USE(resolver->Resolve(context, result)); + } } // Runs after the user callback (or its returned promise) settles. @@ -808,52 +807,6 @@ void LockManager::OnEnvironmentCleanup(void* arg) { LockManager::GetCurrent()->CleanupEnvironment(env); } -static MaybeLocal CreateLockInfoObject(Isolate* isolate, - Local context, - const std::u16string& name, - Lock::Mode mode, - const std::string& client_id) { - Local obj = Object::New(isolate); - Environment* env = Environment::GetCurrent(context); - - // TODO(ilyasshabi): Add ToV8Value that directly accepts std::u16string - // so we can avoid the manual String::NewFromTwoByte() - Local name_string; - if (!String::NewFromTwoByte(isolate, - reinterpret_cast(name.data()), - NewStringType::kNormal, - static_cast(name.length())) - .ToLocal(&name_string)) { - return MaybeLocal(); - } - if (obj->Set(context, env->name_string(), name_string).IsNothing()) { - return MaybeLocal(); - } - - Local mode_string; - if (!String::NewFromUtf8( - isolate, - mode == Lock::Mode::Exclusive ? kExclusiveMode : kSharedMode) - .ToLocal(&mode_string)) { - return MaybeLocal(); - } - if (obj->Set(context, env->mode_string(), mode_string).IsNothing()) { - return MaybeLocal(); - } - - Local client_id_string; - if (!String::NewFromUtf8(isolate, client_id.c_str()) - .ToLocal(&client_id_string)) { - return MaybeLocal(); - } - if (obj->Set(context, env->client_id_string(), client_id_string) - .IsNothing()) { - return MaybeLocal(); - } - - return obj; -} - LockManager LockManager::current_; void CreatePerIsolateProperties(IsolateData* isolate_data, @@ -862,21 +815,17 @@ void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethod(isolate, target, "request", LockManager::Request); SetMethod(isolate, target, "query", LockManager::Query); - Local shared_mode; - if (String::NewFromUtf8(isolate, kSharedMode).ToLocal(&shared_mode)) { - target->Set(FIXED_ONE_BYTE_STRING(isolate, "LOCK_MODE_SHARED"), - shared_mode); - } - Local exclusive_mode; - if (String::NewFromUtf8(isolate, kExclusiveMode).ToLocal(&exclusive_mode)) { - target->Set(FIXED_ONE_BYTE_STRING(isolate, "LOCK_MODE_EXCLUSIVE"), - exclusive_mode); - } - Local stolen_error; - if (String::NewFromUtf8(isolate, kLockStolenError).ToLocal(&stolen_error)) { - target->Set(FIXED_ONE_BYTE_STRING(isolate, "LOCK_STOLEN_ERROR"), - stolen_error); - } + PropertyAttribute read_only = static_cast( + PropertyAttribute::ReadOnly | PropertyAttribute::DontDelete); + target->Set(FIXED_ONE_BYTE_STRING(isolate, "LOCK_MODE_SHARED"), + FIXED_ONE_BYTE_STRING(isolate, "shared"), + read_only); + target->Set(FIXED_ONE_BYTE_STRING(isolate, "LOCK_MODE_EXCLUSIVE"), + FIXED_ONE_BYTE_STRING(isolate, "exclusive"), + read_only); + target->Set(FIXED_ONE_BYTE_STRING(isolate, "LOCK_STOLEN_ERROR"), + FIXED_ONE_BYTE_STRING(isolate, "LOCK_STOLEN"), + read_only); } void CreatePerContextProperties(Local target, diff --git a/src/util-inl.h b/src/util-inl.h index da9268dcf2ff43..4e9c4fea85d12e 100644 --- a/src/util-inl.h +++ b/src/util-inl.h @@ -356,6 +356,26 @@ v8::MaybeLocal ToV8Value(v8::Local context, .FromMaybe(v8::Local()); } +v8::MaybeLocal ToV8Value(v8::Local context, + std::u16string_view str, + v8::Isolate* isolate) { + if (isolate == nullptr) isolate = context->GetIsolate(); + if (str.length() >= static_cast(v8::String::kMaxLength)) + [[unlikely]] { + // V8 only has a TODO comment about adding an exception when the maximum + // string size is exceeded. + ThrowErrStringTooLong(isolate); + return v8::MaybeLocal(); + } + + return v8::String::NewFromTwoByte( + isolate, + reinterpret_cast(str.data()), + v8::NewStringType::kNormal, + str.length()) + .FromMaybe(v8::Local()); +} + v8::MaybeLocal ToV8Value(v8::Local context, v8_inspector::StringView str, v8::Isolate* isolate) { diff --git a/src/util.h b/src/util.h index f942cfe4ef285c..c361cefa9a7381 100644 --- a/src/util.h +++ b/src/util.h @@ -681,6 +681,9 @@ inline v8::Maybe FromV8Array(v8::Local context, inline v8::MaybeLocal ToV8Value(v8::Local context, std::string_view str, v8::Isolate* isolate = nullptr); +inline v8::MaybeLocal ToV8Value(v8::Local context, + std::u16string_view str, + v8::Isolate* isolate = nullptr); inline v8::MaybeLocal ToV8Value(v8::Local context, v8_inspector::StringView str, v8::Isolate* isolate); From 875aff9384bd0062cda388dc09b0b520123064c7 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Sun, 28 Sep 2025 08:31:47 -0700 Subject: [PATCH 2/2] src: make additional cleanups in node locks impl * Track memory held by the Lock instance * Clean up some Utf8/TwoByteString handling --- src/node_locks.cc | 53 +++++++++++++++++++++++++---------------------- src/node_locks.h | 10 ++++++--- src/util.h | 9 ++++++++ 3 files changed, 44 insertions(+), 28 deletions(-) diff --git a/src/node_locks.cc b/src/node_locks.cc index f26f112f3524d2..9c871569c4d46b 100644 --- a/src/node_locks.cc +++ b/src/node_locks.cc @@ -23,12 +23,10 @@ using v8::Isolate; using v8::Local; using v8::LocalVector; using v8::MaybeLocal; -using v8::NewStringType; using v8::Object; using v8::ObjectTemplate; using v8::Promise; using v8::PropertyAttribute; -using v8::String; using v8::Value; // Reject two promises and return `false` on failure. @@ -51,19 +49,26 @@ Lock::Lock(Environment* env, released_promise_.Reset(env_->isolate(), released); } +void Lock::MemoryInfo(node::MemoryTracker* tracker) const { + tracker->TrackFieldWithSize("name", name_.size()); + tracker->TrackField("client_id", client_id_); + tracker->TrackField("waiting_promise", waiting_promise_); + tracker->TrackField("released_promise", released_promise_); +} + LockRequest::LockRequest(Environment* env, Local waiting, Local released, Local callback, const std::u16string& name, Lock::Mode mode, - const std::string& client_id, + std::string client_id, bool steal, bool if_available) : env_(env), name_(name), mode_(mode), - client_id_(client_id), + client_id_(std::move(client_id)), steal_(steal), if_available_(if_available) { waiting_promise_.Reset(env_->isolate(), waiting); @@ -85,6 +90,7 @@ Local GetLockInfoTemplate(Environment* env) { return tmpl; } +// The request here can be either a Lock or a LockRequest. static MaybeLocal CreateLockInfoObject(Environment* env, const auto& request) { auto tmpl = GetLockInfoTemplate(env); @@ -573,22 +579,13 @@ void LockManager::Request(const FunctionCallbackInfo& args) { CHECK(args[4]->IsBoolean()); // ifAvailable CHECK(args[5]->IsFunction()); // callback - Local resource_name_str = args[0].As(); - TwoByteValue resource_name_utf16(isolate, resource_name_str); - std::u16string resource_name( - reinterpret_cast(*resource_name_utf16), - resource_name_utf16.length()); - String::Utf8Value client_id_utf8(isolate, args[1]); - std::string client_id(*client_id_utf8); - String::Utf8Value mode_utf8(isolate, args[2]); - std::string mode_str(*mode_utf8); + TwoByteValue resource_name(isolate, args[0]); + Utf8Value client_id(isolate, args[1]); + Utf8Value mode(isolate, args[2]); bool steal = args[3]->BooleanValue(isolate); bool if_available = args[4]->BooleanValue(isolate); Local callback = args[5].As(); - Lock::Mode lock_mode = - mode_str == "shared" ? Lock::Mode::Shared : Lock::Mode::Exclusive; - Local waiting_promise; Local released_promise; @@ -611,15 +608,17 @@ void LockManager::Request(const FunctionCallbackInfo& args) { env->AddCleanupHook(LockManager::OnEnvironmentCleanup, env); } - auto lock_request = std::make_unique(env, - waiting_promise, - released_promise, - callback, - resource_name, - lock_mode, - client_id, - steal, - if_available); + auto lock_request = std::make_unique( + env, + waiting_promise, + released_promise, + callback, + resource_name.ToU16String(), + mode.ToStringView() == "shared" ? Lock::Mode::Shared + : Lock::Mode::Exclusive, + client_id.ToString(), + steal, + if_available); // Steal requests get priority by going to front of queue if (steal) { manager->pending_queue_.emplace_front(std::move(lock_request)); @@ -842,6 +841,10 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(OnIfAvailableReject); } +void LockHolder::MemoryInfo(node::MemoryTracker* tracker) const { + tracker->TrackField("lock", lock_); +} + BaseObjectPtr LockHolder::Create(Environment* env, std::shared_ptr lock) { Local obj; diff --git a/src/node_locks.h b/src/node_locks.h index 68a2d84ff38419..cee1d26a42ff94 100644 --- a/src/node_locks.h +++ b/src/node_locks.h @@ -15,7 +15,7 @@ namespace node::worker::locks { -class Lock final { +class Lock final : public MemoryRetainer { public: enum class Mode { Shared, Exclusive }; @@ -53,6 +53,10 @@ class Lock final { return released_promise_.Get(env_->isolate()); } + void MemoryInfo(node::MemoryTracker* tracker) const override; + SET_MEMORY_INFO_NAME(Lock) + SET_SELF_SIZE(Lock) + private: Environment* env_; std::u16string name_; @@ -79,7 +83,7 @@ class LockHolder final : public BaseObject { std::shared_ptr lock() const { return lock_; } - SET_NO_MEMORY_INFO() + void MemoryInfo(node::MemoryTracker* tracker) const override; SET_MEMORY_INFO_NAME(LockHolder) SET_SELF_SIZE(LockHolder) @@ -101,7 +105,7 @@ class LockRequest final { v8::Local callback, const std::u16string& name, Lock::Mode mode, - const std::string& client_id, + std::string client_id, bool steal, bool if_available); ~LockRequest() = default; diff --git a/src/util.h b/src/util.h index c361cefa9a7381..6dfccf3a342da3 100644 --- a/src/util.h +++ b/src/util.h @@ -562,6 +562,15 @@ class Utf8Value : public MaybeStackBuffer { class TwoByteValue : public MaybeStackBuffer { public: explicit TwoByteValue(v8::Isolate* isolate, v8::Local value); + + inline std::u16string ToU16String() const { + return std::u16string(reinterpret_cast(out()), length()); + } + + inline std::u16string_view ToU16StringView() const { + return std::u16string_view(reinterpret_cast(out()), + length()); + } }; class BufferValue : public MaybeStackBuffer {