src: fix live lock between environments with blocked requests

Project: nodejs/node | Language: C++/JavaScript | Concepts: concurrency, error-handling, testing-strategy, observability
Difficulty: mid
Link: https://github.com/nodejs/node/pull/65520

The problem

The LockManager in Node.js would wake up every environment (env) with pending lock requests, even if none of those requests could actually proceed. If two environments each had a request blocked on the same resource, they would repeatedly wake each other up without making progress—a classic livelock.

Why it's non-trivial

A naive solution might simply wake all environments with pending requests, but this doesn't account for whether the requests are actually grantable. This can cause environments to continuously wake each other without any request ever being fulfilled, wasting CPU and potentially stalling the system.

The approach

1. Only wake environments if their request can make progress

Previously, the code would wake all environments with pending requests:

-        // Collect unique environments to wake up later
-        if (request->env() != env) {
-          other_envs_to_wake.insert(request->env());
-        }

Old code: Always adds other environments to other_envs_to_wake if they have any pending request, regardless of whether it can proceed.

The fix changes this logic to only wake environments if their request is grantable or uses ifAvailable (which must always call its callback):

+        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;
+        }

New code: Only adds another environment if its request can actually be granted or is an ifAvailable request.

2. Add observability for wake events

A usage counter is incremented whenever an environment is woken, which helps with debugging and testing:

+  COUNT_GENERIC_USAGE("LockManager.WakeEnvironment");

This line adds a counter to track how often environments are woken up, aiding in regression tests and observability.

3. Regression test to prove the fix

A new test is added to ensure that a livelock does not occur:

+  (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());

This test ensures that after a second blocked request is queued, the number of environment wake events does not increase, proving that unnecessary wakeups are avoided.

From the review thread

No notable review discussion on this PR.

Takeaways

• Only wake or notify other threads/processes if there is a real chance for progress; avoid unnecessary wakeups to prevent livelock.
• Use observability tools (like counters or logs) to verify and enforce concurrency invariants in tests.
• Regression tests that assert on side effects (like wake counts) can prevent subtle concurrency bugs from returning.