From 769b088b84b4ffde994778653ebde645c82aec56 Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 24 Aug 2026 19:36:44 +0200 Subject: [PATCH] src: fix live lock between environments with blocked requests Signed-off-by: ishabi --- src/node_locks.cc | 25 ++++++---- test/parallel/test-web-locks-no-livelock.js | 51 +++++++++++++++++++++ 2 files changed, 67 insertions(+), 9 deletions(-) create mode 100644 test/parallel/test-web-locks-no-livelock.js diff --git a/src/node_locks.cc b/src/node_locks.cc index 26fac6b62c4c..4fca3a7df94e 100644 --- a/src/node_locks.cc +++ b/src/node_locks.cc @@ -2,6 +2,7 @@ #include "base_object-inl.h" #include "env-inl.h" +#include "node_debug.h" #include "node_errors.h" #include "node_external_reference.h" #include "node_internals.h" @@ -237,8 +238,8 @@ void LockManager::ProcessQueue(Environment* env) { * 1- Build first_seen_for_resource: the oldest pending request * for every resource name we encounter * 2- Decide what to do with each entry: - * – If it belongs to another Environment, remember that env so we - * can wake it later + * – If it belongs to another Environment, remember that env only if + * the request can make progress * – For our Environment, pick one of: * * grantable_request – can be granted now * * if_available_request – user asked for ifAvailable and the @@ -255,11 +256,6 @@ void LockManager::ProcessQueue(Environment* env) { ++queue_iter) { LockRequest* request = queue_iter->get(); - // Collect unique environments to wake up later - if (request->env() != env) { - other_envs_to_wake.insert(request->env()); - } - // During a single pass, the first time we see a resource name is the // earliest pending request auto& first_for_resource = first_seen_for_resource[request->name()]; @@ -283,12 +279,21 @@ void LockManager::ProcessQueue(Environment* env) { // If both are shared, they're compatible and can proceed } - // Only process requests from the current environment + bool is_grantable = + !should_wait_for_earlier_requests && IsGrantable(request); + + // Requests must be processed on their Environment's event loop. Wake + // another Environment only when it has a request that can make + // progress; otherwise blocked Environments would continuously wake + // each other. if (request->env() != env) { + if (is_grantable || request->if_available()) { + other_envs_to_wake.insert(request->env()); + } continue; } - if (should_wait_for_earlier_requests || !IsGrantable(request)) { + if (!is_grantable) { if (request->if_available()) { // ifAvailable request when resource not available: grant with null if_available_request = std::move(*queue_iter); @@ -754,6 +759,8 @@ void LockManager::ReleaseLock(Lock* lock) { void LockManager::WakeEnvironment(Environment* target_env) { if (target_env == nullptr || target_env->is_stopping()) return; + COUNT_GENERIC_USAGE("LockManager.WakeEnvironment"); + // Schedule ProcessQueue in the target Environment on its event loop. target_env->SetImmediateThreadsafe([](Environment* env_to_wake) { if (env_to_wake != nullptr && !env_to_wake->is_stopping()) { diff --git a/test/parallel/test-web-locks-no-livelock.js b/test/parallel/test-web-locks-no-livelock.js new file mode 100644 index 000000000000..d654acb2b353 --- /dev/null +++ b/test/parallel/test-web-locks-no-livelock.js @@ -0,0 +1,51 @@ +// Flags: --expose-internals +// Regression test for https://github.com/nodejs/node/issues/62644. +'use strict'; + +const common = require('../common'); +if (!common.isDebug) { + common.skip('Only works in debug mode'); +} + +const assert = require('node:assert'); +const { once } = require('node:events'); +const { + isMainThread, + parentPort, + workerData, + Worker, +} = require('node:worker_threads'); + +const resource = `web-locks-no-livelock`; + +if (!isMainThread) { + // A pending lock request alone does not keep the worker alive, so keep a + // message listener until the parent terminates this Environment. + parentPort.on('message', () => {}); + navigator.locks.request(workerData, () => {}); + parentPort.postMessage('ready'); +} else { + const { internalBinding } = require('internal/test/binding'); + const { getGenericUsageCount } = internalBinding('debug'); + const wakeCounter = 'LockManager.WakeEnvironment'; + + (async () => { + let worker; + let pendingInMain; + await navigator.locks.request(resource, common.mustCall(async () => { + // The worker queues an exclusive request for the resource held above. + worker = new Worker(__filename, { workerData: resource }); + await once(worker, 'message'); + + // Queueing a second blocked request must not wake the worker: its + // request cannot make progress, so the two Environments would wake + // each other forever (livelock). + const wakeCount = getGenericUsageCount(wakeCounter); + pendingInMain = navigator.locks.request(resource, common.mustCall()); + assert.strictEqual(getGenericUsageCount(wakeCounter), wakeCount); + })); + + await pendingInMain; + await worker.terminate(); + })().then(common.mustCall()); +}