From b0a20bd3330206e069ab2ea9de449fbb669d8e50 Mon Sep 17 00:00:00 2001 From: ishabi Date: Fri, 30 May 2025 00:57:31 +0200 Subject: [PATCH 01/16] worker: add web locks api --- doc/api/globals.md | 52 ++ doc/api/worker_threads.md | 140 ++++ .../bootstrap/web/exposed-window-or-worker.js | 3 + lib/internal/locks.js | 275 +++++++ lib/internal/navigator.js | 12 + lib/worker_threads.js | 3 + node.gyp | 2 + src/async_wrap.h | 1 + src/node_binding.cc | 1 + src/node_binding.h | 1 + src/node_external_reference.h | 1 + src/node_locks.cc | 684 ++++++++++++++++++ src/node_locks.h | 142 ++++ test/fixtures/wpt/README.md | 1 + test/fixtures/wpt/interfaces/web-locks.idl | 45 ++ test/fixtures/wpt/versions.json | 4 + test/fixtures/wpt/web-locks/META.yml | 5 + test/fixtures/wpt/web-locks/README.md | 5 + test/fixtures/wpt/web-locks/WEB_FEATURES.yml | 3 + .../wpt/web-locks/acquire.https.any.js | 136 ++++ .../bfcache/abort.tentative.https.html | 64 ++ .../bfcache/held.tentative.https.html | 45 ++ .../fixtures/wpt/web-locks/bfcache/helpers.js | 15 + ...release-across-thread.tentative.https.html | 46 ++ .../bfcache/release.tentative.https.html | 48 ++ ...sharedworker-multiple.tentative.https.html | 69 ++ .../wpt/web-locks/clientids.https.html | 45 ++ .../after-worker-termination.https.html | 20 + .../crashtests/iframe-append-2.https.html | 18 + .../crashtests/iframe-append.https.html | 21 + .../crashtests/settle-after-steal.https.html | 10 + .../crashtests/worker-termination.https.html | 16 + test/fixtures/wpt/web-locks/frames.https.html | 265 +++++++ test/fixtures/wpt/web-locks/held.https.any.js | 91 +++ .../wpt/web-locks/idlharness.https.any.js | 29 + .../wpt/web-locks/ifAvailable.https.any.js | 163 +++++ .../web-locks/lock-attributes.https.any.js | 18 + .../wpt/web-locks/mode-exclusive.https.any.js | 34 + .../wpt/web-locks/mode-mixed.https.any.js | 98 +++ .../wpt/web-locks/mode-shared.https.any.js | 38 + .../wpt/web-locks/non-fully-active.https.html | 73 ++ .../wpt/web-locks/non-secure-context.any.js | 14 + .../wpt/web-locks/opaque-origin.https.html | 87 +++ ...partitioned-web-locks.tentative.https.html | 172 +++++ .../wpt/web-locks/query-empty.https.any.js | 18 + .../wpt/web-locks/query-ordering.https.html | 131 ++++ .../fixtures/wpt/web-locks/query.https.any.js | 227 ++++++ .../wpt/web-locks/resource-names.https.any.js | 56 ++ .../wpt/web-locks/resources/helpers.js | 91 +++ .../web-locks/resources/iframe-parent.html | 37 + .../wpt/web-locks/resources/iframe.html | 52 ++ .../wpt/web-locks/resources/parentworker.js | 10 + .../resources/partitioned-parent.html | 30 + .../wpt/web-locks/resources/service-worker.js | 7 + .../resources/sw-controlled-iframe.html | 35 + .../wpt/web-locks/resources/worker.js | 56 ++ .../wpt/web-locks/secure-context.https.any.js | 14 + .../wpt/web-locks/signal.https.any.js | 261 +++++++ .../fixtures/wpt/web-locks/steal.https.any.js | 91 +++ .../storage-buckets.tentative.https.any.js | 56 ++ .../fixtures/wpt/web-locks/workers.https.html | 122 ++++ test/parallel/test-bootstrap-modules.js | 1 + test/parallel/test-web-locks-query.js | 92 +++ test/parallel/test-web-locks.js | 142 ++++ test/sequential/test-async-wrap-getasyncid.js | 1 + test/wpt/status/web-locks.json | 20 + test/wpt/test-web-locks.js | 9 + tools/doc/type-parser.mjs | 4 + typings/globals.d.ts | 2 + typings/internalBinding/locks.d.ts | 31 + 70 files changed, 4611 insertions(+) create mode 100644 lib/internal/locks.js create mode 100644 src/node_locks.cc create mode 100644 src/node_locks.h create mode 100644 test/fixtures/wpt/interfaces/web-locks.idl create mode 100644 test/fixtures/wpt/web-locks/META.yml create mode 100644 test/fixtures/wpt/web-locks/README.md create mode 100644 test/fixtures/wpt/web-locks/WEB_FEATURES.yml create mode 100644 test/fixtures/wpt/web-locks/acquire.https.any.js create mode 100644 test/fixtures/wpt/web-locks/bfcache/abort.tentative.https.html create mode 100644 test/fixtures/wpt/web-locks/bfcache/held.tentative.https.html create mode 100644 test/fixtures/wpt/web-locks/bfcache/helpers.js create mode 100644 test/fixtures/wpt/web-locks/bfcache/release-across-thread.tentative.https.html create mode 100644 test/fixtures/wpt/web-locks/bfcache/release.tentative.https.html create mode 100644 test/fixtures/wpt/web-locks/bfcache/sharedworker-multiple.tentative.https.html create mode 100644 test/fixtures/wpt/web-locks/clientids.https.html create mode 100644 test/fixtures/wpt/web-locks/crashtests/after-worker-termination.https.html create mode 100644 test/fixtures/wpt/web-locks/crashtests/iframe-append-2.https.html create mode 100644 test/fixtures/wpt/web-locks/crashtests/iframe-append.https.html create mode 100644 test/fixtures/wpt/web-locks/crashtests/settle-after-steal.https.html create mode 100644 test/fixtures/wpt/web-locks/crashtests/worker-termination.https.html create mode 100644 test/fixtures/wpt/web-locks/frames.https.html create mode 100644 test/fixtures/wpt/web-locks/held.https.any.js create mode 100644 test/fixtures/wpt/web-locks/idlharness.https.any.js create mode 100644 test/fixtures/wpt/web-locks/ifAvailable.https.any.js create mode 100644 test/fixtures/wpt/web-locks/lock-attributes.https.any.js create mode 100644 test/fixtures/wpt/web-locks/mode-exclusive.https.any.js create mode 100644 test/fixtures/wpt/web-locks/mode-mixed.https.any.js create mode 100644 test/fixtures/wpt/web-locks/mode-shared.https.any.js create mode 100644 test/fixtures/wpt/web-locks/non-fully-active.https.html create mode 100644 test/fixtures/wpt/web-locks/non-secure-context.any.js create mode 100644 test/fixtures/wpt/web-locks/opaque-origin.https.html create mode 100644 test/fixtures/wpt/web-locks/partitioned-web-locks.tentative.https.html create mode 100644 test/fixtures/wpt/web-locks/query-empty.https.any.js create mode 100644 test/fixtures/wpt/web-locks/query-ordering.https.html create mode 100644 test/fixtures/wpt/web-locks/query.https.any.js create mode 100644 test/fixtures/wpt/web-locks/resource-names.https.any.js create mode 100644 test/fixtures/wpt/web-locks/resources/helpers.js create mode 100644 test/fixtures/wpt/web-locks/resources/iframe-parent.html create mode 100644 test/fixtures/wpt/web-locks/resources/iframe.html create mode 100644 test/fixtures/wpt/web-locks/resources/parentworker.js create mode 100644 test/fixtures/wpt/web-locks/resources/partitioned-parent.html create mode 100644 test/fixtures/wpt/web-locks/resources/service-worker.js create mode 100644 test/fixtures/wpt/web-locks/resources/sw-controlled-iframe.html create mode 100644 test/fixtures/wpt/web-locks/resources/worker.js create mode 100644 test/fixtures/wpt/web-locks/secure-context.https.any.js create mode 100644 test/fixtures/wpt/web-locks/signal.https.any.js create mode 100644 test/fixtures/wpt/web-locks/steal.https.any.js create mode 100644 test/fixtures/wpt/web-locks/storage-buckets.tentative.https.any.js create mode 100644 test/fixtures/wpt/web-locks/workers.https.html create mode 100644 test/parallel/test-web-locks-query.js create mode 100644 test/parallel/test-web-locks.js create mode 100644 test/wpt/status/web-locks.json create mode 100644 test/wpt/test-web-locks.js create mode 100644 typings/internalBinding/locks.d.ts diff --git a/doc/api/globals.md b/doc/api/globals.md index 769e5aca37a67f..0de73e58e9414d 100644 --- a/doc/api/globals.md +++ b/doc/api/globals.md @@ -768,6 +768,55 @@ consisting of the runtime name and major version number. console.log(`The user-agent is ${navigator.userAgent}`); // Prints "Node.js/21" ``` +### `navigator.locks` + + + +> Stability: 1 - Experimental + +The `navigator.locks` read-only property returns a [`LockManager`][] instance that +can be used to coordinate access to resources that may be shared across multiple +threads within the same process. This global implementation matches the semantics +of the [browser `LockManager`][] API. + +```mjs +// Request an exclusive lock +await navigator.locks.request('my_resource', async (lock) => { + // The lock has been acquired. + console.log(`Lock acquired: ${lock.name}`); + // Lock is automatically released when the function returns +}); + +// Request a shared lock +await navigator.locks.request('shared_resource', { mode: 'shared' }, async (lock) => { + // Multiple shared locks can be held simultaneously + console.log(`Shared lock acquired: ${lock.name}`); +}); +``` + +```cjs +// Request an exclusive lock +navigator.locks.request('my_resource', async (lock) => { + // The lock has been acquired. + console.log(`Lock acquired: ${lock.name}`); + // Lock is automatically released when the function returns +}).then(() => { + console.log('Lock released'); +}); + +// Request a shared lock +navigator.locks.request('shared_resource', { mode: 'shared' }, async (lock) => { + // Multiple shared locks can be held simultaneously + console.log(`Shared lock acquired: ${lock.name}`); +}).then(() => { + console.log('Shared lock released'); +}); +``` + +See [`worker.locks`][] for detailed API documentation. + ## Class: `PerformanceEntry` + +> Stability: 1 - Experimental + +* {LockManager} + +An instance of a [`LockManager`][LockManager] that can be used to coordinate +access to resources that may be shared across multiple threads within the same +process. The API mirrors the semantics of the +[browser `LockManager`][] + +### Class: `Lock` + + + +The `Lock` interface provides information about a lock that has been granted via +[`locks.request()`][locks.request()] + +#### `lock.name` + + + +* {string} + +The name of the lock. + +#### `lock.mode` + + + +* {string} + +The mode of the lock. Either `shared` or `exclusive`. + +### Class: `LockManager` + + + +The `LockManager` interface provides methods for requesting and introspecting +locks. To obtain a `LockManager` instance use `require('node:worker_threads').locks`. + +This implementation matches the [browser `LockManager`][] API. + +#### `locks.request(name[, options], callback)` + + + +* `name` {string} +* `options` {Object} + * `mode` {string} Either `'exclusive'` or `'shared'`. **Default:** `'exclusive'`. + * `ifAvailable` {boolean} If `true`, the request will only be granted if the + lock is not already held. If it cannot be granted, `callback` will be + invoked with `null` instead of a `Lock` instance. **Default:** `false`. + * `steal` {boolean} If `true`, any existing locks with the same name are + released and the request is granted immediately, pre-empting any queued + requests. **Default:** `false`. + * `signal` {AbortSignal} that can be used to abort a + pending (but not yet granted) lock request. +* `callback` {Function} Invoked once the lock is granted (or immediately with + `null` if `ifAvailable` is `true` and the lock is unavailable). The lock is + released automatically when the function returns, or—if the function returns + a promise—when that promise settles. +* Returns: {Promise} Resolves once the lock has been released. + +```mjs +import { locks } from 'node:worker_threads'; + +await locks.request('my_resource', async (lock) => { + // The lock has been acquired. +}); +// The lock has been released here. +``` + +```cjs +'use strict'; + +const { locks } = require('node:worker_threads'); + +locks.request('my_resource', async (lock) => { + // The lock has been acquired. +}).then(() => { + // The lock has been released here. +}); +``` + +#### `locks.query()` + + + +* Returns: {Promise} + +Resolves with a `LockManagerSnapshot` describing the currently held and pending +locks for the current process. + +```mjs +import { locks } from 'node:worker_threads'; + +const snapshot = await locks.query(); +for (const lock of snapshot.held) { + console.log(`held lock: name ${lock.name}, mode ${lock.mode}`); +} +for (const pending of snapshot.pending) { + console.log(`pending lock: name ${pending.name}, mode ${pending.mode}`); +} +``` + +```cjs +'use strict'; + +const { locks } = require('node:worker_threads'); + +locks.query().then((snapshot) => { + for (const lock of snapshot.held) { + console.log(`held lock: name ${lock.name}, mode ${lock.mode}`); + } + for (const pending of snapshot.pending) { + console.log(`pending lock: name ${pending.name}, mode ${pending.mode}`); + } +}); +``` + ## Class: `BroadcastChannel extends EventTarget` + + + + + + + + + diff --git a/test/fixtures/wpt/web-locks/query-empty.https.any.js b/test/fixtures/wpt/web-locks/query-empty.https.any.js new file mode 100644 index 00000000000000..88ffdb7f810d6d --- /dev/null +++ b/test/fixtures/wpt/web-locks/query-empty.https.any.js @@ -0,0 +1,18 @@ +// META: title=Web Locks API: navigator.locks.query method - no locks held +// META: script=resources/helpers.js +// META: global=window,dedicatedworker,sharedworker,serviceworker + +'use strict'; + +promise_test(async t => { + const state = await navigator.locks.query(); + + assert_own_property(state, 'pending', 'State has `pending` property'); + assert_true(Array.isArray(state.pending), + 'State `pending` property is an array'); + assert_array_equals(state.pending, [], 'Pending array is empty'); + + assert_own_property(state, 'held', 'State has `held` property'); + assert_true(Array.isArray(state.held), 'State `held` property is an array'); + assert_array_equals(state.held, [], 'Held array is empty'); +}, 'query() returns dictionary with empty arrays when no locks are held'); diff --git a/test/fixtures/wpt/web-locks/query-ordering.https.html b/test/fixtures/wpt/web-locks/query-ordering.https.html new file mode 100644 index 00000000000000..d5e722baf75b6b --- /dev/null +++ b/test/fixtures/wpt/web-locks/query-ordering.https.html @@ -0,0 +1,131 @@ + + +Web Locks API: navigator.locks.query ordering + + + + + + diff --git a/test/fixtures/wpt/web-locks/query.https.any.js b/test/fixtures/wpt/web-locks/query.https.any.js new file mode 100644 index 00000000000000..14fdeca7a426c4 --- /dev/null +++ b/test/fixtures/wpt/web-locks/query.https.any.js @@ -0,0 +1,227 @@ +// META: title=Web Locks API: navigator.locks.query method +// META: script=resources/helpers.js + +'use strict'; + +// Returns an array of the modes for the locks with matching name. +function modes(list, name) { + return list.filter(item => item.name === name).map(item => item.mode); +} +// Returns an array of the clientIds for the locks with matching name. +function clients(list, name) { + return list.filter(item => item.name === name).map(item => item.clientId); +} + +promise_test(async t => { + const res = uniqueName(t); + + await navigator.locks.request(res, async lock1 => { + // Attempt to request this again - should be blocked. + let lock2_acquired = false; + navigator.locks.request(res, lock2 => { lock2_acquired = true; }); + + // Verify that it was blocked. + await navigator.locks.request(res, {ifAvailable: true}, async lock3 => { + assert_false(lock2_acquired, 'second request should be blocked'); + assert_equals(lock3, null, 'third request should have failed'); + + const state = await navigator.locks.query(); + + assert_own_property(state, 'pending', 'State has `pending` property'); + assert_true(Array.isArray(state.pending), + 'State `pending` property is an array'); + const pending_info = state.pending[0]; + assert_own_property(pending_info, 'name', + 'Pending info dictionary has `name` property'); + assert_own_property(pending_info, 'mode', + 'Pending info dictionary has `mode` property'); + assert_own_property(pending_info, 'clientId', + 'Pending info dictionary has `clientId` property'); + + assert_own_property(state, 'held', 'State has `held` property'); + assert_true(Array.isArray(state.held), + 'State `held` property is an array'); + const held_info = state.held[0]; + assert_own_property(held_info, 'name', + 'Held info dictionary has `name` property'); + assert_own_property(held_info, 'mode', + 'Held info dictionary has `mode` property'); + assert_own_property(held_info, 'clientId', + 'Held info dictionary has `clientId` property'); + }); + }); +}, 'query() returns dictionaries with expected properties'); + + + +promise_test(async t => { + const res = uniqueName(t); + + await navigator.locks.request(res, async lock1 => { + const state = await navigator.locks.query(); + assert_array_equals(modes(state.held, res), ['exclusive'], + 'Held lock should appear once'); + }); + + await navigator.locks.request(res, {mode: 'shared'}, async lock1 => { + const state = await navigator.locks.query(); + assert_array_equals(modes(state.held, res), ['shared'], + 'Held lock should appear once'); + }); +}, 'query() reports individual held locks'); + +promise_test(async t => { + const res1 = uniqueName(t); + const res2 = uniqueName(t); + + await navigator.locks.request(res1, async lock1 => { + await navigator.locks.request(res2, {mode: 'shared'}, async lock2 => { + const state = await navigator.locks.query(); + assert_array_equals(modes(state.held, res1), ['exclusive'], + 'Held lock should appear once'); + assert_array_equals(modes(state.held, res2), ['shared'], + 'Held lock should appear once'); + }); + }); +}, 'query() reports multiple held locks'); + +promise_test(async t => { + const res = uniqueName(t); + + await navigator.locks.request(res, async lock1 => { + // Attempt to request this again - should be blocked. + let lock2_acquired = false; + navigator.locks.request(res, lock2 => { lock2_acquired = true; }); + + // Verify that it was blocked. + await navigator.locks.request(res, {ifAvailable: true}, async lock3 => { + assert_false(lock2_acquired, 'second request should be blocked'); + assert_equals(lock3, null, 'third request should have failed'); + + const state = await navigator.locks.query(); + assert_array_equals(modes(state.pending, res), ['exclusive'], + 'Pending lock should appear once'); + assert_array_equals(modes(state.held, res), ['exclusive'], + 'Held lock should appear once'); + }); + }); +}, 'query() reports pending and held locks'); + +promise_test(async t => { + const res = uniqueName(t); + + await navigator.locks.request(res, {mode: 'shared'}, async lock1 => { + await navigator.locks.request(res, {mode: 'shared'}, async lock2 => { + const state = await navigator.locks.query(); + assert_array_equals(modes(state.held, res), ['shared', 'shared'], + 'Held lock should appear twice'); + }); + }); +}, 'query() reports held shared locks with appropriate count'); + +promise_test(async t => { + const res = uniqueName(t); + + await navigator.locks.request(res, async lock1 => { + let lock2_acquired = false, lock3_acquired = false; + navigator.locks.request(res, {mode: 'shared'}, + lock2 => { lock2_acquired = true; }); + navigator.locks.request(res, {mode: 'shared'}, + lock3 => { lock3_acquired = true; }); + + await navigator.locks.request(res, {ifAvailable: true}, async lock4 => { + assert_equals(lock4, null, 'lock should not be available'); + assert_false(lock2_acquired, 'second attempt should be blocked'); + assert_false(lock3_acquired, 'third attempt should be blocked'); + + const state = await navigator.locks.query(); + assert_array_equals(modes(state.held, res), ['exclusive'], + 'Held lock should appear once'); + + assert_array_equals(modes(state.pending, res), ['shared', 'shared'], + 'Pending lock should appear twice'); + }); + }); +}, 'query() reports pending shared locks with appropriate count'); + +promise_test(async t => { + const res1 = uniqueName(t); + const res2 = uniqueName(t); + + await navigator.locks.request(res1, async lock1 => { + await navigator.locks.request(res2, async lock2 => { + const state = await navigator.locks.query(); + + const res1_clients = clients(state.held, res1); + const res2_clients = clients(state.held, res2); + + assert_equals(res1_clients.length, 1, 'Each lock should have one holder'); + assert_equals(res2_clients.length, 1, 'Each lock should have one holder'); + + assert_array_equals(res1_clients, res2_clients, + 'Both locks should have same clientId'); + }); + }); +}, 'query() reports the same clientId for held locks from the same context'); + +promise_test(async t => { + const res = uniqueName(t); + + const worker = new Worker('resources/worker.js'); + t.add_cleanup(() => { worker.terminate(); }); + + await postToWorkerAndWait( + worker, {op: 'request', name: res, mode: 'shared'}); + + await navigator.locks.request(res, {mode: 'shared'}, async lock => { + const state = await navigator.locks.query(); + const res_clients = clients(state.held, res); + assert_equals(res_clients.length, 2, 'Clients should have same resource'); + assert_not_equals(res_clients[0], res_clients[1], + 'Clients should have different ids'); + }); +}, 'query() reports different ids for held locks from different contexts'); + +promise_test(async t => { + const res1 = uniqueName(t); + const res2 = uniqueName(t); + + const worker = new Worker('resources/worker.js'); + t.add_cleanup(() => { worker.terminate(); }); + + // Acquire 1 in the worker. + await postToWorkerAndWait(worker, {op: 'request', name: res1}) + + // Acquire 2 here. + await new Promise(resolve => { + navigator.locks.request(res2, lock => { + resolve(); + return new Promise(() => {}); // Never released. + }); + }); + + // Request 2 in the worker. + postToWorkerAndWait(worker, {op: 'request', name: res2}); + assert_true((await postToWorkerAndWait(worker, { + op: 'request', name: res2, ifAvailable: true + })).failed, 'Lock request should have failed'); + + // Request 1 here. + navigator.locks.request( + res1, t.unreached_func('Lock should not be acquired')); + + // Verify that we're seeing a deadlock. + const state = await navigator.locks.query(); + const res1_held_clients = clients(state.held, res1); + const res2_held_clients = clients(state.held, res2); + const res1_pending_clients = clients(state.pending, res1); + const res2_pending_clients = clients(state.pending, res2); + + assert_equals(res1_held_clients.length, 1); + assert_equals(res2_held_clients.length, 1); + assert_equals(res1_pending_clients.length, 1); + assert_equals(res2_pending_clients.length, 1); + + assert_equals(res1_held_clients[0], res2_pending_clients[0]); + assert_equals(res2_held_clients[0], res1_pending_clients[0]); +}, 'query() can observe a deadlock'); diff --git a/test/fixtures/wpt/web-locks/resource-names.https.any.js b/test/fixtures/wpt/web-locks/resource-names.https.any.js new file mode 100644 index 00000000000000..1031b3f7ba2e34 --- /dev/null +++ b/test/fixtures/wpt/web-locks/resource-names.https.any.js @@ -0,0 +1,56 @@ +// META: title=Web Locks API: Resources DOMString edge cases +// META: global=window,dedicatedworker,sharedworker,serviceworker + +'use strict'; + +function code_points(s) { + return [...s] + .map(c => '0x' + c.charCodeAt(0).toString(16).toUpperCase()) + .join(' '); +} + +[ + '', // Empty strings + 'abc\x00def', // Embedded NUL + '\uD800', // Unpaired low surrogage + '\uDC00', // Unpaired high surrogage + '\uDC00\uD800', // Swapped surrogate pair + '\uFFFF' // Non-character +].forEach(string => { + promise_test(async t => { + await navigator.locks.request(string, lock => { + assert_equals(lock.name, string, + 'Requested name matches granted name'); + }); + }, 'DOMString: ' + code_points(string)); +}); + +promise_test(async t => { + // '\uD800' treated as a USVString would become '\uFFFD'. + await navigator.locks.request('\uD800', async lock => { + assert_equals(lock.name, '\uD800'); + + // |lock| is held for the duration of this name. It + // Should not block acquiring |lock2| with a distinct + // DOMString. + await navigator.locks.request('\uFFFD', lock2 => { + assert_equals(lock2.name, '\uFFFD'); + }); + + // If we did not time out, this passed. + }); +}, 'Resource names that are not valid UTF-16 are not mangled'); + +promise_test(async t => { + for (const name of ['-', '-foo']) { + await promise_rejects_dom( + t, 'NotSupportedError', + navigator.locks.request(name, lock => {}), + 'Names starting with "-" should be rejected'); + } + let got_lock = false; + await navigator.locks.request('x-anything', lock => { + got_lock = true; + }); + assert_true(got_lock, 'Names with embedded "-" should be accepted'); +}, 'Names cannot start with "-"'); diff --git a/test/fixtures/wpt/web-locks/resources/helpers.js b/test/fixtures/wpt/web-locks/resources/helpers.js new file mode 100644 index 00000000000000..3fb89711ab7a29 --- /dev/null +++ b/test/fixtures/wpt/web-locks/resources/helpers.js @@ -0,0 +1,91 @@ +// Test helpers used by multiple Web Locks API tests. +(() => { + + // Generate a unique resource identifier, using the script path and + // test case name. This is useful to avoid lock interference between + // test cases. + let res_num = 0; + self.uniqueName = (testCase, prefix) => { + return `${self.location.pathname}-${prefix}-${testCase.name}-${++res_num}`; + }; + self.uniqueNameByQuery = () => { + const prefix = new URL(location.href).searchParams.get('prefix'); + return `${prefix}-${++res_num}`; + } + + // Inject an iframe showing the given url into the page, and resolve + // the returned promise when the frame is loaded. + self.iframe = url => new Promise(resolve => { + const element = document.createElement('iframe'); + element.addEventListener( + 'load', () => { resolve(element); }, { once: true }); + element.src = url; + document.documentElement.appendChild(element); + }); + + // Post a message to the target frame, and resolve the returned + // promise when a response comes back. The posted data is annotated + // with unique id to track the response. This assumes the use of + // 'iframe.html' as the frame, which implements this protocol. + let next_request_id = 0; + self.postToFrameAndWait = (frame, data) => { + const iframe_window = frame.contentWindow; + data.rqid = next_request_id++; + iframe_window.postMessage(data, '*'); + return new Promise(resolve => { + const listener = event => { + if (event.source !== iframe_window || event.data.rqid !== data.rqid) + return; + self.removeEventListener('message', listener); + resolve(event.data); + }; + self.addEventListener('message', listener); + }); + }; + + // Post a message to the target worker, and resolve the returned + // promise when a response comes back. The posted data is annotated + // with unique id to track the response. This assumes the use of + // 'worker.js' as the worker, which implements this protocol. + self.postToWorkerAndWait = (worker, data) => { + return new Promise(resolve => { + data.rqid = next_request_id++; + worker.postMessage(data); + const listener = event => { + if (event.data.rqid !== data.rqid) + return; + worker.removeEventListener('message', listener); + resolve(event.data); + }; + worker.addEventListener('message', listener); + }); + }; + + /** + * Request a lock and hold it until the subtest ends. + * @param {*} t test runner object + * @param {string} name lock name + * @param {LockOptions=} options lock options + * @returns + */ + self.requestLockAndHold = (t, name, options = {}) => { + let [promise, resolve] = self.makePromiseAndResolveFunc(); + const released = navigator.locks.request(name, options, () => promise); + // Add a cleanup function that releases the lock by resolving the promise, + // and then waits until the lock is really released, to avoid contaminating + // following tests with temporarily held locks. + t.add_cleanup(() => { + resolve(); + // Cleanup shouldn't fail if the request is aborted. + return released.catch(() => undefined); + }); + return released; + }; + + self.makePromiseAndResolveFunc = () => { + let resolve; + const promise = new Promise(r => { resolve = r; }); + return [promise, resolve]; + }; + +})(); diff --git a/test/fixtures/wpt/web-locks/resources/iframe-parent.html b/test/fixtures/wpt/web-locks/resources/iframe-parent.html new file mode 100644 index 00000000000000..ec63045b4a0561 --- /dev/null +++ b/test/fixtures/wpt/web-locks/resources/iframe-parent.html @@ -0,0 +1,37 @@ + +Helper IFrame + diff --git a/test/fixtures/wpt/web-locks/resources/iframe.html b/test/fixtures/wpt/web-locks/resources/iframe.html new file mode 100644 index 00000000000000..ba63c77bae50d7 --- /dev/null +++ b/test/fixtures/wpt/web-locks/resources/iframe.html @@ -0,0 +1,52 @@ + +Helper IFrame + diff --git a/test/fixtures/wpt/web-locks/resources/parentworker.js b/test/fixtures/wpt/web-locks/resources/parentworker.js new file mode 100644 index 00000000000000..2b2b2c20280c5c --- /dev/null +++ b/test/fixtures/wpt/web-locks/resources/parentworker.js @@ -0,0 +1,10 @@ +// Just transparently forwards things to the child worker + +importScripts("/web-locks/resources/helpers.js"); +const worker = new Worker("/web-locks/resources/worker.js"); + +self.addEventListener("message", async ev => { + const data = await postToWorkerAndWait(worker, ev.data); + data.rqid = ev.data.rqid; + postMessage(data); +}); diff --git a/test/fixtures/wpt/web-locks/resources/partitioned-parent.html b/test/fixtures/wpt/web-locks/resources/partitioned-parent.html new file mode 100644 index 00000000000000..ec19c8dbaa69bb --- /dev/null +++ b/test/fixtures/wpt/web-locks/resources/partitioned-parent.html @@ -0,0 +1,30 @@ + + + + diff --git a/test/fixtures/wpt/web-locks/resources/service-worker.js b/test/fixtures/wpt/web-locks/resources/service-worker.js new file mode 100644 index 00000000000000..027863e33e0457 --- /dev/null +++ b/test/fixtures/wpt/web-locks/resources/service-worker.js @@ -0,0 +1,7 @@ +// Responds to '/clientId' with the request's clientId. +self.addEventListener('fetch', e => { + if (new URL(e.request.url).pathname === '/clientId') { + e.respondWith(new Response(JSON.stringify({clientId: e.clientId}))); + return; + } +}); diff --git a/test/fixtures/wpt/web-locks/resources/sw-controlled-iframe.html b/test/fixtures/wpt/web-locks/resources/sw-controlled-iframe.html new file mode 100644 index 00000000000000..bc5c9bdb838257 --- /dev/null +++ b/test/fixtures/wpt/web-locks/resources/sw-controlled-iframe.html @@ -0,0 +1,35 @@ + + +iframe used in clientId test + diff --git a/test/fixtures/wpt/web-locks/resources/worker.js b/test/fixtures/wpt/web-locks/resources/worker.js new file mode 100644 index 00000000000000..cc71631ba6fa22 --- /dev/null +++ b/test/fixtures/wpt/web-locks/resources/worker.js @@ -0,0 +1,56 @@ +'use strict'; + +// Map of id => function that releases a lock. + +const held = new Map(); +let next_lock_id = 1; + +function processMessage(e) { + const target = this; + + function respond(data) { + target.postMessage(Object.assign(data, {rqid: e.data.rqid})); + } + + switch (e.data.op) { + case 'request': { + const controller = new AbortController(); + navigator.locks.request( + e.data.name, { + mode: e.data.mode || 'exclusive', + ifAvailable: e.data.ifAvailable || false, + signal: e.data.abortImmediately ? controller.signal : undefined, + }, lock => { + if (lock === null) { + respond({ack: 'request', failed: true}); + return; + } + let lock_id = next_lock_id++; + let release; + const promise = new Promise(r => { release = r; }); + held.set(lock_id, release); + respond({ack: 'request', lock_id: lock_id}); + return promise; + }).catch(e => { + respond({ack: 'request', error: e.name}); + }); + if (e.data.abortImmediately) { + controller.abort(); + } + break; + } + + case 'release': + held.get(e.data.lock_id)(); + held.delete(e.data.lock_id); + respond({ack: 'release', lock_id: e.data.lock_id}); + break; + } +} + +self.addEventListener('message', processMessage); + +self.addEventListener('connect', ev => { + // Shared worker case + ev.ports[0].onmessage = processMessage; +}); diff --git a/test/fixtures/wpt/web-locks/secure-context.https.any.js b/test/fixtures/wpt/web-locks/secure-context.https.any.js new file mode 100644 index 00000000000000..29ae7aea475f2d --- /dev/null +++ b/test/fixtures/wpt/web-locks/secure-context.https.any.js @@ -0,0 +1,14 @@ +// META: title=Web Locks API: API requires secure context +// META: global=window,dedicatedworker,sharedworker,serviceworker + +'use strict'; + +test(t => { + assert_true(self.isSecureContext); + assert_idl_attribute(navigator, 'locks', + 'navigator.locks exists in secure context'); + assert_true('LockManager' in self, + 'LockManager is present in secure contexts'); + assert_true('Lock' in self, + 'Lock interface is present in secure contexts'); +}, 'API presence in secure contexts'); diff --git a/test/fixtures/wpt/web-locks/signal.https.any.js b/test/fixtures/wpt/web-locks/signal.https.any.js new file mode 100644 index 00000000000000..5a37e3ae87182e --- /dev/null +++ b/test/fixtures/wpt/web-locks/signal.https.any.js @@ -0,0 +1,261 @@ +// META: title=Web Locks API: AbortSignal integration +// META: script=resources/helpers.js +// META: global=window,dedicatedworker,sharedworker,serviceworker + +'use strict'; + +promise_test(async t => { + const res = uniqueName(t); + + // These cases should not work: + for (const signal of ['string', 12.34, false, {}, Symbol(), () => {}, self]) { + await promise_rejects_js( + t, TypeError, + navigator.locks.request( + res, {signal}, t.unreached_func('callback should not run')), + 'Bindings should throw if the signal option is a not an AbortSignal'); + } +}, 'The signal option must be an AbortSignal'); + +promise_test(async t => { + const res = uniqueName(t); + const controller = new AbortController(); + controller.abort(); + + await promise_rejects_dom( + t, 'AbortError', + navigator.locks.request(res, {signal: controller.signal}, + t.unreached_func('callback should not run')), + 'Request should reject with AbortError'); +}, 'Passing an already aborted signal aborts'); + +promise_test(async t => { + const res = uniqueName(t); + + const controller = new AbortController(); + const reason = 'My dog ate it.'; + controller.abort(reason); + + const promise = + navigator.locks.request(res, {signal: controller.signal}, + t.unreached_func('callback should not run')); + + await promise_rejects_exactly( + t, reason, promise, "Rejection should give the abort reason"); +}, 'Passing an already aborted signal rejects with the custom abort reason.'); + +promise_test(async t => { + const res = uniqueName(t); + + const controller = new AbortController(); + controller.abort(); + + const promise = + navigator.locks.request(res, {signal: controller.signal}, + t.unreached_func('callback should not run')); + + await promise_rejects_exactly( + t, controller.signal.reason, promise, + "Rejection should give the abort reason"); +}, 'Passing an already aborted signal rejects with the default abort reason.'); + +promise_test(async t => { + const res = uniqueName(t); + + // Grab a lock and hold it until this subtest completes. + requestLockAndHold(t, res); + + const controller = new AbortController(); + + const promise = + navigator.locks.request(res, {signal: controller.signal}, + t.unreached_func('callback should not run')); + + // Verify the request is enqueued: + const state = await navigator.locks.query(); + assert_equals(state.held.filter(lock => lock.name === res).length, 1, + 'Number of held locks'); + assert_equals(state.pending.filter(lock => lock.name === res).length, 1, + 'Number of pending locks'); + + const rejected = promise_rejects_dom( + t, 'AbortError', promise, 'Request should reject with AbortError'); + + controller.abort(); + + await rejected; + +}, 'An aborted request results in AbortError'); + +promise_test(async t => { + const res = uniqueName(t); + + // Grab a lock and hold it until this subtest completes. + requestLockAndHold(t, res); + + const controller = new AbortController(); + + const promise = + navigator.locks.request(res, {signal: controller.signal}, lock => {}); + + // Verify the request is enqueued: + const state = await navigator.locks.query(); + assert_equals(state.held.filter(lock => lock.name === res).length, 1, + 'Number of held locks'); + assert_equals(state.pending.filter(lock => lock.name === res).length, 1, + 'Number of pending locks'); + + const rejected = promise_rejects_dom( + t, 'AbortError', promise, 'Request should reject with AbortError'); + + let callback_called = false; + t.step_timeout(() => { + callback_called = true; + controller.abort(); + }, 10); + + await rejected; + assert_true(callback_called, 'timeout should have caused the abort'); + +}, 'Abort after a timeout'); + +promise_test(async t => { + const res = uniqueName(t); + + const controller = new AbortController(); + + let got_lock = false; + await navigator.locks.request( + res, {signal: controller.signal}, async lock => { got_lock = true; }); + + assert_true(got_lock, 'Lock should be acquired if abort is not signaled.'); + +}, 'Signal that is not aborted'); + +promise_test(async t => { + const res = uniqueName(t); + + const controller = new AbortController(); + + let got_lock = false; + const p = navigator.locks.request( + res, {signal: controller.signal}, lock => { got_lock = true; }); + + // Even though lock is grantable, this abort should be processed synchronously. + controller.abort(); + + await promise_rejects_dom(t, 'AbortError', p, 'Request should abort'); + + assert_false(got_lock, 'Request should be aborted if signal is synchronous'); + + await navigator.locks.request(res, lock => { got_lock = true; }); + assert_true(got_lock, 'Subsequent request should not be blocked'); + +}, 'Synchronously signaled abort'); + +promise_test(async t => { + const res = uniqueName(t); + + const controller = new AbortController(); + + // Make a promise that resolves when the lock is acquired. + const [acquired_promise, acquired_func] = makePromiseAndResolveFunc(); + + // Request the lock. + let release_func; + const released_promise = navigator.locks.request( + res, {signal: controller.signal}, lock => { + acquired_func(); + + // Hold lock until release_func is called. + const [waiting_promise, waiting_func] = makePromiseAndResolveFunc(); + release_func = waiting_func; + return waiting_promise; + }); + + // Wait for the lock to be acquired. + await acquired_promise; + + // Signal an abort. + controller.abort(); + + // Release the lock. + release_func('resolved ok'); + + assert_equals(await released_promise, 'resolved ok', + 'Lock released promise should not reject'); + +}, 'Abort signaled after lock granted'); + +promise_test(async t => { + const res = uniqueName(t); + + const controller = new AbortController(); + + // Make a promise that resolves when the lock is acquired. + const [acquired_promise, acquired_func] = makePromiseAndResolveFunc(); + + // Request the lock. + let release_func; + const released_promise = navigator.locks.request( + res, {signal: controller.signal}, lock => { + acquired_func(); + + // Hold lock until release_func is called. + const [waiting_promise, waiting_func] = makePromiseAndResolveFunc(); + release_func = waiting_func; + return waiting_promise; + }); + + // Wait for the lock to be acquired. + await acquired_promise; + + // Release the lock. + release_func('resolved ok'); + + // Signal an abort. + controller.abort(); + + assert_equals(await released_promise, 'resolved ok', + 'Lock released promise should not reject'); + +}, 'Abort signaled after lock released'); + +promise_test(async t => { + const res = uniqueName(t); + + const controller = new AbortController(); + const first = requestLockAndHold(t, res, { signal: controller.signal }); + const next = navigator.locks.request(res, () => "resolved"); + controller.abort(); + + await promise_rejects_dom(t, "AbortError", first, "Request should abort"); + assert_equals( + await next, + "resolved", + "The next request is processed after abort" + ); +}, "Abort should process the next pending lock request"); + +promise_test(async t => { + const res = uniqueName(t); + + const controller = new AbortController(); + const promise = requestLockAndHold(t, res, { signal: controller.signal }); + + const reason = "My cat handled it"; + controller.abort(reason); + + await promise_rejects_exactly(t, reason, promise, "Rejection should give the abort reason"); +}, "Aborted promise should reject with the custom abort reason"); + +promise_test(async t => { + const res = uniqueName(t); + + const controller = new AbortController(); + const promise = requestLockAndHold(t, res, { signal: controller.signal }); + + controller.abort(); + + await promise_rejects_exactly(t, controller.signal.reason, promise, "Should be the same reason"); +}, "Aborted promise should reject with the default abort reason"); diff --git a/test/fixtures/wpt/web-locks/steal.https.any.js b/test/fixtures/wpt/web-locks/steal.https.any.js new file mode 100644 index 00000000000000..d165b9d179fb14 --- /dev/null +++ b/test/fixtures/wpt/web-locks/steal.https.any.js @@ -0,0 +1,91 @@ +// META: title=Web Locks API: steal option +// META: script=resources/helpers.js +// META: global=window,dedicatedworker,sharedworker,serviceworker + +'use strict'; + +const never_settled = new Promise(resolve => { /* never */ }); + +promise_test(async t => { + const res = uniqueName(t); + let callback_called = false; + await navigator.locks.request(res, {steal: true}, lock => { + callback_called = true; + assert_not_equals(lock, null, 'Lock should be granted'); + }); + assert_true(callback_called, 'Callback should be called'); +}, 'Lock available'); + +promise_test(async t => { + const res = uniqueName(t); + let callback_called = false; + + // Grab and hold the lock. + navigator.locks.request(res, lock => never_settled).catch(_ => {}); + + // Steal it. + await navigator.locks.request(res, {steal: true}, lock => { + callback_called = true; + assert_not_equals(lock, null, 'Lock should be granted'); + }); + + assert_true(callback_called, 'Callback should be called'); +}, 'Lock not available'); + +promise_test(async t => { + const res = uniqueName(t); + + // Grab and hold the lock. + const promise = navigator.locks.request(res, lock => never_settled); + const assertion = promise_rejects_dom( + t, 'AbortError', promise, `Initial request's promise should reject`); + + // Steal it. + await navigator.locks.request(res, {steal: true}, lock => {}); + + await assertion; + +}, `Broken lock's release promise rejects`); + +promise_test(async t => { + const res = uniqueName(t); + + // Grab and hold the lock. + navigator.locks.request(res, lock => never_settled).catch(_ => {}); + + // Make a request for it. + let request_granted = false; + const promise = navigator.locks.request(res, lock => { + request_granted = true; + }); + + // Steal it. + await navigator.locks.request(res, {steal: true}, lock => { + assert_false(request_granted, 'Steal should override request'); + }); + + await promise; + assert_true(request_granted, 'Request should eventually be granted'); + +}, `Requested lock's release promise is deferred`); + +promise_test(async t => { + const res = uniqueName(t); + + // Grab and hold the lock. + navigator.locks.request(res, lock => never_settled).catch(_ => {}); + + // Steal it. + let saw_abort = false; + const first_steal = navigator.locks.request( + res, {steal: true}, lock => never_settled).catch(error => { + saw_abort = true; + }); + + // Steal it again. + await navigator.locks.request(res, {steal: true}, lock => {}); + + await first_steal; + assert_true(saw_abort, 'First steal should have aborted'); + +}, 'Last caller wins'); diff --git a/test/fixtures/wpt/web-locks/storage-buckets.tentative.https.any.js b/test/fixtures/wpt/web-locks/storage-buckets.tentative.https.any.js new file mode 100644 index 00000000000000..a6b4f59a95d715 --- /dev/null +++ b/test/fixtures/wpt/web-locks/storage-buckets.tentative.https.any.js @@ -0,0 +1,56 @@ +// META: title=Web Locks API: Storage Buckets have independent lock sets +// META: script=resources/helpers.js +// META: script=/storage/buckets/resources/util.js +// META: global=window,dedicatedworker,sharedworker,serviceworker + +'use strict'; + +/** + * Returns whether bucket1 and bucket2 share locks + * @param {*} t test runner object + * @param {*} bucket1 Storage bucket + * @param {*} bucket2 Storage bucket + */ +async function locksAreShared(t, bucket1, bucket2) { + const lock_name = self.uniqueName(t); + let callback_called = false; + let locks_are_shared; + await bucket1.locks.request(lock_name, async lock => { + await bucket2.locks.request( + lock_name, { ifAvailable: true }, async lock => { + callback_called = true; + locks_are_shared = lock == null; + }); + }); + assert_true(callback_called, 'callback should be called'); + return locks_are_shared; +} + +promise_test(async t => { + await prepareForBucketTest(t); + + const inboxBucket = await navigator.storageBuckets.open('inbox'); + const draftsBucket = await navigator.storageBuckets.open('drafts'); + + assert_true( + await locksAreShared(t, navigator, navigator), + 'The default bucket should share locks with itself'); + + assert_true( + await locksAreShared(t, inboxBucket, inboxBucket), + 'A non default bucket should share locks with itself'); + + assert_false( + await locksAreShared(t, navigator, inboxBucket), + 'The default bucket shouldn\'t share locks with a non default bucket'); + + assert_false( + await locksAreShared(t, draftsBucket, inboxBucket), + 'Two different non default buckets shouldn\'t share locks'); + + const inboxBucket2 = await navigator.storageBuckets.open('inbox'); + + assert_true( + await self.locksAreShared(t, inboxBucket, inboxBucket2), + 'A two instances of the same non default bucket should share locks with theirselves'); +}, 'Storage buckets have independent locks'); diff --git a/test/fixtures/wpt/web-locks/workers.https.html b/test/fixtures/wpt/web-locks/workers.https.html new file mode 100644 index 00000000000000..9fe38dbe38c798 --- /dev/null +++ b/test/fixtures/wpt/web-locks/workers.https.html @@ -0,0 +1,122 @@ + + +Web Locks API: Workers + + + + + diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index caf3b315f78872..deffc73ffb6993 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -149,6 +149,7 @@ if (isMainThread) { 'NativeModule internal/streams/writable', 'NativeModule internal/worker', 'NativeModule internal/worker/io', + 'NativeModule internal/worker/locks', 'NativeModule internal/worker/messaging', 'NativeModule stream', 'NativeModule stream/promises', diff --git a/test/parallel/test-web-locks-query.js b/test/parallel/test-web-locks-query.js new file mode 100644 index 00000000000000..962abd92f130dd --- /dev/null +++ b/test/parallel/test-web-locks-query.js @@ -0,0 +1,92 @@ +'use strict'; + +require('../common'); +const { describe, it } = require('node:test'); +const assert = require('node:assert'); +const { Worker } = require('worker_threads'); + +describe('Web Locks - query missing WPT tests', () => { + it('should report different ids for held locks from different contexts', async () => { + const worker = new Worker(` + const { parentPort } = require('worker_threads'); + + navigator.locks.request('different-contexts-resource', { mode: 'shared' }, async (lock) => { + const state = await navigator.locks.query(); + const heldLocks = state.held.filter(l => l.name === 'different-contexts-resource'); + + parentPort.postMessage({ clientId: heldLocks[0].clientId }); + + await new Promise(resolve => { + parentPort.once('message', () => resolve()); + }); + }).catch(err => parentPort.postMessage({ error: err.message })); + `, { eval: true }); + + const workerResult = await new Promise((resolve) => { + worker.once('message', resolve); + }); + + await navigator.locks.request('different-contexts-resource', { mode: 'shared' }, async (lock) => { + const state = await navigator.locks.query(); + const heldLocks = state.held.filter((l) => l.name === 'different-contexts-resource'); + + const mainClientId = heldLocks[0].clientId; + + assert.notStrictEqual(mainClientId, workerResult.clientId); + + worker.postMessage('release'); + }); + + await worker.terminate(); + }); + + it('should observe a deadlock scenario', async () => { + const worker = new Worker(` + const { parentPort } = require('worker_threads'); + + navigator.locks.request('deadlock-resource-1', async (lock1) => { + parentPort.postMessage({ acquired: 'resource1' }); + + await new Promise(resolve => { + parentPort.once('message', () => resolve()); + }); + + const result = await navigator.locks.request('deadlock-resource-2', + { ifAvailable: true }, (lock2) => lock2 !== null); + + parentPort.postMessage({ acquired2: result }); + + await new Promise(resolve => { + parentPort.once('message', () => resolve()); + }); + }).catch(err => parentPort.postMessage({ error: err.message })); + `, { eval: true }); + + const step1 = await new Promise((resolve) => { + worker.once('message', resolve); + }); + assert.strictEqual(step1.acquired, 'resource1'); + + await navigator.locks.request('deadlock-resource-2', async (lock2) => { + worker.postMessage('try-resource2'); + + const step2 = await new Promise((resolve) => { + worker.once('message', resolve); + }); + assert.strictEqual(step2.acquired2, false); + + const canGetResource1 = await navigator.locks.request('deadlock-resource-1', + { ifAvailable: true }, (lock1) => lock1 !== null); + + assert.strictEqual(canGetResource1, false); + + const state = await navigator.locks.query(); + const resource2Lock = state.held.find((l) => l.name === 'deadlock-resource-2'); + assert(resource2Lock); + + worker.postMessage('release'); + }); + + await worker.terminate(); + }); +}); diff --git a/test/parallel/test-web-locks.js b/test/parallel/test-web-locks.js new file mode 100644 index 00000000000000..1eb6777b1dab09 --- /dev/null +++ b/test/parallel/test-web-locks.js @@ -0,0 +1,142 @@ +'use strict'; + +require('../common'); +const { describe, it } = require('node:test'); +const assert = require('node:assert'); +const { Worker } = require('worker_threads'); + +describe('Web Locks with worker threads', () => { + it('should handle exclusive locks', async () => { + const worker = new Worker(` + const { parentPort } = require('worker_threads'); + const assert = require('node:assert'); + + navigator.locks.request('exclusive-test', async (lock) => { + assert.strictEqual(lock.mode, 'exclusive'); + parentPort.postMessage({ success: true }); + }).catch(err => parentPort.postMessage({ error: err.message })); + `, { eval: true }); + + const result = await new Promise((resolve) => { + worker.once('message', resolve); + }); + + assert.strictEqual(result.success, true); + await worker.terminate(); + + await navigator.locks.request('exclusive-test', async (lock) => { + assert.strictEqual(lock.mode, 'exclusive'); + assert.strictEqual(lock.name, 'exclusive-test'); + }); + }); + + it('should handle shared locks', async () => { + const worker = new Worker(` + const { parentPort } = require('worker_threads'); + const assert = require('node:assert'); + + navigator.locks.request('shared-test', { mode: 'shared' }, async (lock) => { + assert.strictEqual(lock.mode, 'shared'); + parentPort.postMessage({ success: true }); + }).catch(err => parentPort.postMessage({ error: err.message })); + `, { eval: true }); + + const result = await new Promise((resolve) => { + worker.once('message', resolve); + }); + assert.strictEqual(result.success, true); + + await navigator.locks.request('shared-test', { mode: 'shared' }, async (lock1) => { + await navigator.locks.request('shared-test', { mode: 'shared' }, async (lock2) => { + assert.strictEqual(lock1.mode, 'shared'); + assert.strictEqual(lock2.mode, 'shared'); + }); + }); + + await worker.terminate(); + }); + + it('should handle steal option - no existing lock', async () => { + await navigator.locks.request('steal-simple', { steal: true }, async (lock) => { + assert.strictEqual(lock.name, 'steal-simple'); + assert.strictEqual(lock.mode, 'exclusive'); + }); + }); + + it('should handle steal option - existing lock', async () => { + let originalLockRejected = false; + + const originalLockPromise = navigator.locks.request('steal-target', async (lock) => { + assert.strictEqual(lock.name, 'steal-target'); + return 'original-completed'; + }).catch((err) => { + originalLockRejected = true; + assert.strictEqual(err.name, 'AbortError'); + assert.strictEqual(err.message, 'The operation was aborted'); + return 'original-rejected'; + }); + + const stealResult = await navigator.locks.request('steal-target', { steal: true }, async (stolenLock) => { + assert.strictEqual(stolenLock.name, 'steal-target'); + assert.strictEqual(stolenLock.mode, 'exclusive'); + return 'steal-completed'; + }); + + assert.strictEqual(stealResult, 'steal-completed'); + + const originalResult = await originalLockPromise; + assert.strictEqual(originalLockRejected, true); + assert.strictEqual(originalResult, 'original-rejected'); + }); + + it('should handle ifAvailable option', async () => { + await navigator.locks.request('ifavailable-test', async () => { + const result = await navigator.locks.request('ifavailable-test', { ifAvailable: true }, (lock) => { + return lock; // should be null + }); + + assert.strictEqual(result, null); + + const availableResult = await navigator.locks.request('ifavailable-different-resource', + { ifAvailable: true }, (lock) => { + return lock !== null; + }); + + assert.strictEqual(availableResult, true); + }); + }); + + it('should handle AbortSignal', async () => { + const worker = new Worker(` + const { parentPort } = require('worker_threads'); + const assert = require('node:assert'); + + const controller = new AbortController(); + + navigator.locks.request('signal-after-grant', { signal: controller.signal }, async (lock) => { + parentPort.postMessage({ acquired: true }); + + setTimeout(() => controller.abort(), 50); + + await new Promise(resolve => setTimeout(resolve, 100)); + return 'completed successfully'; + }).then(result => { + parentPort.postMessage({ resolved: result }); + }).catch(err => { + parentPort.postMessage({ rejected: err.name }); + }); + `, { eval: true }); + + const acquired = await new Promise((resolve) => { + worker.once('message', resolve); + }); + assert.strictEqual(acquired.acquired, true); + + const result = await new Promise((resolve) => { + worker.once('message', resolve); + }); + assert.strictEqual(result.resolved, 'completed successfully'); + + await worker.terminate(); + }); +}); diff --git a/test/sequential/test-async-wrap-getasyncid.js b/test/sequential/test-async-wrap-getasyncid.js index a75207b66e6633..5568cde0bc2a3a 100644 --- a/test/sequential/test-async-wrap-getasyncid.js +++ b/test/sequential/test-async-wrap-getasyncid.js @@ -71,6 +71,7 @@ const { getSystemErrorName } = require('util'); delete providers.QUIC_ENDPOINT; delete providers.QUIC_SESSION; delete providers.QUIC_STREAM; + delete providers.LOCKS; const objKeys = Object.keys(providers); if (objKeys.length > 0) diff --git a/test/wpt/status/web-locks.json b/test/wpt/status/web-locks.json new file mode 100644 index 00000000000000..8a547c6c416959 --- /dev/null +++ b/test/wpt/status/web-locks.json @@ -0,0 +1,20 @@ +{ + "non-secure-context.any.js": { + "skip": "navigator.locks is only present in secure contexts" + }, + "query.https.any.js": { + "fail": { + "expected": [ + "query() reports different ids for held locks from different contexts", + "query() can observe a deadlock" + ], + "note": "Browser-specific test" + } + }, + "secure-context.https.any.js": { + "skip": "Different secure context behavior in Node.js" + }, + "storage-buckets.tentative.https.any.js": { + "skip": "Node.js does not implement Storage Buckets API" + } +} diff --git a/test/wpt/test-web-locks.js b/test/wpt/test-web-locks.js new file mode 100644 index 00000000000000..f7080a23757de5 --- /dev/null +++ b/test/wpt/test-web-locks.js @@ -0,0 +1,9 @@ +'use strict'; + +const { WPTRunner } = require('../common/wpt'); + +// Run serially to avoid cross-test interference on the shared LockManager. +const runner = new WPTRunner('web-locks', { concurrency: 1 }); + +runner.pretendGlobalThisAs('Window'); +runner.runJsTests(); diff --git a/tools/doc/type-parser.mjs b/tools/doc/type-parser.mjs index 1a54eb29d15f27..b6d0a69d46743b 100644 --- a/tools/doc/type-parser.mjs +++ b/tools/doc/type-parser.mjs @@ -333,6 +333,10 @@ const customTypesMap = { 'quic.OnHeadersCallback': 'quic.html#callback-onheaderscallback', 'quic.OnTrailersCallback': 'quic.html#callback-ontrailerscallback', 'quic.OnPullCallback': 'quic.html#callback-onpullcallback', + + 'Lock': 'worker_threads.html#class-lock', + 'LockManager': 'worker_threads.html#class-lockmanager', + 'LockManagerSnapshot': 'https://developer.mozilla.org/en-US/docs/Web/API/LockManagerSnapshot', }; const arrayPart = /(?:\[])+$/; diff --git a/typings/globals.d.ts b/typings/globals.d.ts index 1bd3f46d0e2567..7b53a869b9ca11 100644 --- a/typings/globals.d.ts +++ b/typings/globals.d.ts @@ -7,6 +7,7 @@ import { HttpParserBinding } from './internalBinding/http_parser'; import { InspectorBinding } from './internalBinding/inspector'; import { FsBinding } from './internalBinding/fs'; import { FsDirBinding } from './internalBinding/fs_dir'; +import { LocksBinding } from './internalBinding/locks'; import { MessagingBinding } from './internalBinding/messaging'; import { OptionsBinding } from './internalBinding/options'; import { OSBinding } from './internalBinding/os'; @@ -33,6 +34,7 @@ interface InternalBindingMap { fs_dir: FsDirBinding; http_parser: HttpParserBinding; inspector: InspectorBinding; + locks: LocksBinding; messaging: MessagingBinding; modules: ModulesBinding; options: OptionsBinding; diff --git a/typings/internalBinding/locks.d.ts b/typings/internalBinding/locks.d.ts new file mode 100644 index 00000000000000..f0a31d60bd9ac3 --- /dev/null +++ b/typings/internalBinding/locks.d.ts @@ -0,0 +1,31 @@ +declare namespace InternalLocksBinding { + interface LockInfo { + readonly name: string; + readonly mode: 'shared' | 'exclusive'; + readonly clientId: string; + } + + interface LockManagerSnapshot { + readonly held: LockInfo[]; + readonly pending: LockInfo[]; + } + + type LockGrantedCallback = (lock: LockInfo | null) => Promise | any; +} + +export interface LocksBinding { + readonly LOCK_MODE_SHARED: 'shared'; + readonly LOCK_MODE_EXCLUSIVE: 'exclusive'; + readonly LOCK_STOLEN_ERROR: 'LOCK_STOLEN'; + + request( + name: string, + clientId: string, + mode: string, + steal: boolean, + ifAvailable: boolean, + callback: InternalLocksBinding.LockGrantedCallback + ): Promise; + + query(): Promise; +} From 222cfbd72b381ce88f882c69122d36a3ce74ea83 Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 11 Jun 2025 21:10:41 +0200 Subject: [PATCH 02/16] use WebIDL convertors --- lib/internal/locks.js | 124 ++++++++++++++++------------ lib/internal/navigator.js | 4 +- src/node_locks.cc | 140 ++++++++++++++++++++------------ src/node_locks.h | 24 ++++-- test/parallel/test-web-locks.js | 34 ++++++++ 5 files changed, 212 insertions(+), 114 deletions(-) diff --git a/lib/internal/locks.js b/lib/internal/locks.js index beeb592412a5ef..674e2d3cd55105 100644 --- a/lib/internal/locks.js +++ b/lib/internal/locks.js @@ -10,46 +10,78 @@ const { const { ERR_ILLEGAL_CONSTRUCTOR, - ERR_INVALID_ARG_TYPE, - ERR_INVALID_ARG_VALUE, + ERR_INVALID_THIS, } = require('internal/errors'); -const { lazyDOMException } = require('internal/util'); +const { + kEmptyObject, + lazyDOMException, +} = require('internal/util'); const { validateAbortSignal, - validateDictionary, validateFunction, - validateString, } = require('internal/validators'); const { threadId } = require('internal/worker'); +const { + converters, + createEnumConverter, + createDictionaryConverter, +} = require('internal/webidl'); const locks = internalBinding('locks'); const kName = Symbol('kName'); const kMode = Symbol('kMode'); +const kConstructLock = Symbol('kConstructLock'); const kConstructLockManager = Symbol('kConstructLockManager'); +// WebIDL dictionary LockOptions +const convertLockOptions = createDictionaryConverter([ + { + key: 'mode', + converter: createEnumConverter('LockMode', [ + 'shared', + 'exclusive', + ]), + defaultValue: () => 'exclusive', + }, + { + key: 'ifAvailable', + converter: (value) => !!value, + defaultValue: () => false, + }, + { + key: 'steal', + converter: (value) => !!value, + defaultValue: () => false, + }, + { + key: 'signal', + converter: converters.object, + }, +]); + // https://w3c.github.io/web-locks/#api-lock class Lock { - constructor(...args) { - if (!args.length) { + constructor(symbol = undefined, name, mode) { + if (symbol !== kConstructLock) { throw new ERR_ILLEGAL_CONSTRUCTOR(); } - this[kName] = args[0]; - this[kMode] = args[1]; + this[kName] = name; + this[kMode] = mode; } get name() { if (this instanceof Lock) { return this[kName]; } - throw new ERR_INVALID_ARG_TYPE('this', 'Lock', this); + throw new ERR_INVALID_THIS('Lock'); } get mode() { if (this instanceof Lock) { return this[kMode]; } - throw new ERR_INVALID_ARG_TYPE('this', 'Lock', this); + throw new ERR_INVALID_THIS('Lock'); } } @@ -67,7 +99,7 @@ ObjectDefineProperties(Lock.prototype, { // Helper to create Lock objects from internal C++ lock data function createLock(internalLock) { - return internalLock === null ? null : new Lock(internalLock.name, internalLock.mode); + return internalLock === null ? null : new Lock(kConstructLock, internalLock.name, internalLock.mode); } // Convert LOCK_STOLEN_ERROR to AbortError DOMException @@ -80,8 +112,8 @@ function convertLockError(error) { // https://w3c.github.io/web-locks/#api-lock-manager class LockManager { - constructor(...args) { - if (args[0] !== kConstructLockManager) { + constructor(symbol = undefined) { + if (symbol !== kConstructLockManager) { throw new ERR_ILLEGAL_CONSTRUCTOR(); } } @@ -106,21 +138,19 @@ class LockManager { options = undefined; } - validateString(name, 'name'); + name = converters.DOMString(name); validateFunction(callback, 'callback'); - if (options !== undefined) { - validateDictionary(options, 'options'); + if (options === undefined || typeof options === 'function') { + options = kEmptyObject; } - // Set default options - options = { - mode: locks.LOCK_MODE_EXCLUSIVE, - ifAvailable: false, - steal: false, - signal: undefined, - ...options, - }; + // Convert LockOptions dictionary + options = convertLockOptions(options); + + const { mode, ifAvailable, steal, signal } = options; + + validateAbortSignal(signal, 'options.signal'); if (name.startsWith('-')) { // If name starts with U+002D HYPHEN-MINUS (-), then reject promise with a @@ -129,7 +159,7 @@ class LockManager { 'NotSupportedError'); } - if (options.ifAvailable === true && options.steal === true) { + if (ifAvailable === true && steal === true) { // If both options' steal dictionary member and option's // ifAvailable dictionary member are true, then reject promise with a // "NotSupportedError" DOMException. @@ -137,13 +167,7 @@ class LockManager { 'NotSupportedError'); } - if (options.mode !== locks.LOCK_MODE_SHARED && options.mode !== locks.LOCK_MODE_EXCLUSIVE) { - throw new ERR_INVALID_ARG_VALUE('options.mode', - options.mode, - `must be "${locks.LOCK_MODE_SHARED}" or "${locks.LOCK_MODE_EXCLUSIVE}"`); - } - - if (options.mode !== locks.LOCK_MODE_EXCLUSIVE && options.steal === true) { + if (mode !== locks.LOCK_MODE_EXCLUSIVE && steal === true) { // If options' steal dictionary member is true and options' mode // dictionary member is not "exclusive", then return a promise rejected // with a "NotSupportedError" DOMException. @@ -151,8 +175,7 @@ class LockManager { 'NotSupportedError'); } - if (options.signal && - (options.steal === true || options.ifAvailable === true)) { + if (signal && (steal === true || ifAvailable === true)) { // If options' signal dictionary member is present, and either of // options' steal dictionary member or options' ifAvailable dictionary // member is true, then return a promise rejected with a @@ -161,33 +184,28 @@ class LockManager { 'NotSupportedError'); } - if (options.signal !== undefined) { - validateAbortSignal(options.signal, 'options.signal'); - } - - if (options.signal?.aborted) { - throw options.signal.reason || lazyDOMException('The operation was aborted', 'AbortError'); + if (signal) { + signal.throwIfAborted(); } const clientId = `node-${process.pid}-${threadId}`; // Handle requests with AbortSignal - if (options.signal) { + if (signal) { return new Promise((resolve, reject) => { let lockGranted = false; const abortListener = () => { if (!lockGranted) { - reject(options.signal.reason || - lazyDOMException('The operation was aborted', 'AbortError')); + reject(signal.reason || lazyDOMException('The operation was aborted', 'AbortError')); } }; - options.signal.addEventListener('abort', abortListener, { once: true }); + signal.addEventListener('abort', abortListener, { once: true }); const wrappedCallback = (lock) => { return PromiseResolve().then(() => { - if (options.signal.aborted) { + if (signal.aborted) { return undefined; } lockGranted = true; @@ -199,9 +217,9 @@ class LockManager { const released = locks.request( name, clientId, - options.mode, - options.steal, - options.ifAvailable, + mode, + steal, + ifAvailable, wrappedCallback, ); @@ -209,10 +227,10 @@ class LockManager { released .then(resolve, (error) => reject(convertLockError(error))) .finally(() => { - options.signal.removeEventListener('abort', abortListener); + signal.removeEventListener('abort', abortListener); }); } catch (error) { - options.signal.removeEventListener('abort', abortListener); + signal.removeEventListener('abort', abortListener); reject(convertLockError(error)); } }); @@ -226,7 +244,7 @@ class LockManager { // Standard request without signal try { - return await locks.request(name, clientId, options.mode, options.steal, options.ifAvailable, wrapCallback); + return await locks.request(name, clientId, mode, steal, ifAvailable, wrapCallback); } catch (error) { const convertedError = convertLockError(error); throw convertedError; @@ -242,7 +260,7 @@ class LockManager { if (this instanceof LockManager) { return locks.query(); } - throw new ERR_INVALID_ARG_TYPE('this', 'LockManager', this); + throw new ERR_INVALID_THIS('LockManager'); } } diff --git a/lib/internal/navigator.js b/lib/internal/navigator.js index ce0db4439507b8..d85f42d6780e22 100644 --- a/lib/internal/navigator.js +++ b/lib/internal/navigator.js @@ -21,8 +21,6 @@ const { getAvailableParallelism, } = internalBinding('os'); -const { locks } = require('internal/locks'); - const kInitialize = Symbol('kInitialize'); const { @@ -107,7 +105,7 @@ class Navigator { * @return {LockManager} */ get locks() { - this.#locks ??= locks; + this.#locks ??= require('internal/locks').locks; return this.#locks; } diff --git a/src/node_locks.cc b/src/node_locks.cc index ee0541c8a06f00..d35d06947d82df 100644 --- a/src/node_locks.cc +++ b/src/node_locks.cc @@ -7,6 +7,8 @@ #include "util-inl.h" #include "v8.h" +namespace node::worker::locks { + using node::errors::TryCatchScope; using v8::Array; using v8::Context; @@ -23,10 +25,6 @@ using v8::Promise; using v8::String; using v8::Value; -namespace node { -namespace worker { -namespace locks { - static constexpr const char* kSharedMode = "shared"; static constexpr const char* kExclusiveMode = "exclusive"; static constexpr const char* kLockStolenError = "LOCK_STOLEN"; @@ -88,11 +86,11 @@ bool LockManager::IsGrantable(const LockRequest* request) const { if (held_locks_iter == held_locks_.end()) return true; // Exclusive requests cannot coexist with any existing locks - if (request->mode() == Lock::kExclusive) return false; + if (request->mode() == Lock::Mode::Exclusive) return false; // For shared requests, check if any existing lock is exclusive for (const auto& existing_lock : held_locks_iter->second) { - if (existing_lock->mode() == Lock::kExclusive) return false; + if (existing_lock->mode() == Lock::Mode::Exclusive) return false; } // All existing locks are shared, so this shared request can be granted return true; @@ -104,9 +102,10 @@ static void OnLockCallbackSettled( HandleScope handle_scope(info.GetIsolate()); Environment* env = Environment::GetCurrent(info); + // Extract the LockHolder from V8 External data auto* lock_holder = - static_cast*>(info.Data().As()->Value()); - std::shared_ptr lock = *lock_holder; + static_cast(info.Data().As()->Value()); + std::shared_ptr lock = lock_holder->lock(); delete lock_holder; // Release the lock and continue processing the queue. @@ -277,9 +276,12 @@ void LockManager::ProcessQueue(Environment* env) { envs_to_notify.insert(existing_lock->env()); // Immediately reject the stolen lock's released_promise - Local error = - Exception::Error(String::NewFromUtf8(isolate, kLockStolenError) - .ToLocalChecked()); + Local error_string; + if (!String::NewFromUtf8(isolate, kLockStolenError) + .ToLocal(&error_string)) { + return; + } + Local error = Exception::Error(error_string); existing_lock->released_promise()->Reject(context, error).Check(); } @@ -327,6 +329,9 @@ void LockManager::ProcessQueue(Environment* env) { grantable_request->name(), grantable_request->mode(), grantable_request->client_id()); + if (lock_info_obj.IsEmpty()) { + return; + } Local callback_arg = lock_info_obj; Local callback_result; { @@ -340,17 +345,22 @@ void LockManager::ProcessQueue(Environment* env) { grantable_request->released_promise() ->Reject(context, try_catch_scope.Exception()) .Check(); - continue; + return; } } - // Allocate a shared_ptr so the lock remains alive until the callback - // executes. - auto* lock_holder = new std::shared_ptr(granted_lock); - Local on_settled_callback = - Function::New( - context, OnLockCallbackSettled, External::New(isolate, lock_holder)) - .ToLocalChecked(); + // Allocate a LockHolder on the heap to safely manage the lock's lifetime + // until the user's callback promise settles. + // The LockHolder will be deleted in OnLockCallbackSettled. + auto* lock_holder = new LockHolder(granted_lock); + Local on_settled_callback; + if (!Function::New(context, + OnLockCallbackSettled, + External::New(isolate, lock_holder)) + .ToLocal(&on_settled_callback)) { + delete lock_holder; + return; + } // Handle promise chain if (callback_result->IsPromise()) { @@ -423,12 +433,16 @@ void LockManager::Request(const FunctionCallbackInfo& args) { Local callback = args[5].As(); Lock::Mode lock_mode = - mode_str == kSharedMode ? Lock::kShared : Lock::kExclusive; + mode_str == kSharedMode ? Lock::Mode::Shared : Lock::Mode::Exclusive; - Local waiting_promise = - Promise::Resolver::New(context).ToLocalChecked(); - Local released_promise = - Promise::Resolver::New(context).ToLocalChecked(); + Local waiting_promise; + if (!Promise::Resolver::New(context).ToLocal(&waiting_promise)) { + return; + } + Local released_promise; + if (!Promise::Resolver::New(context).ToLocal(&released_promise)) { + return; + } args.GetReturnValue().Set(released_promise->GetPromise()); @@ -467,8 +481,10 @@ void LockManager::Query(const FunctionCallbackInfo& args) { HandleScope scope(isolate); Local context = env->context(); - Local resolver = - Promise::Resolver::New(context).ToLocalChecked(); + Local resolver; + if (!Promise::Resolver::New(context).ToLocal(&resolver)) { + return; + } args.GetReturnValue().Set(resolver->GetPromise()); Local result = Object::New(isolate); @@ -489,6 +505,9 @@ void LockManager::Query(const FunctionCallbackInfo& args) { held_lock->name(), held_lock->mode(), held_lock->client_id()); + if (lock_info.IsEmpty()) { + return; + } held_list->Set(context, index++, lock_info).Check(); } } @@ -503,6 +522,9 @@ void LockManager::Query(const FunctionCallbackInfo& args) { pending_request->name(), pending_request->mode(), pending_request->client_id()); + if (lock_info.IsEmpty()) { + return; + } pending_list->Set(context, index++, lock_info).Check(); } } @@ -621,26 +643,35 @@ static Local CreateLockInfoObject(Isolate* isolate, Lock::Mode mode, const std::string& client_id) { Local obj = Object::New(isolate); - obj->Set( - context, - FIXED_ONE_BYTE_STRING(isolate, "name"), - String::NewFromTwoByte(isolate, - reinterpret_cast(name.data()), - v8::NewStringType::kNormal, - static_cast(name.length())) - .ToLocalChecked()) + + Local name_string; + if (!String::NewFromTwoByte(isolate, + reinterpret_cast(name.data()), + v8::NewStringType::kNormal, + static_cast(name.length())) + .ToLocal(&name_string)) { + return Local(); + } + obj->Set(context, FIXED_ONE_BYTE_STRING(isolate, "name"), name_string) .Check(); - obj->Set(context, - FIXED_ONE_BYTE_STRING(isolate, "mode"), - String::NewFromUtf8( - isolate, mode == Lock::kExclusive ? kExclusiveMode : kSharedMode) - .ToLocalChecked()) + Local mode_string; + if (!String::NewFromUtf8( + isolate, + mode == Lock::Mode::Exclusive ? kExclusiveMode : kSharedMode) + .ToLocal(&mode_string)) { + return Local(); + } + obj->Set(context, FIXED_ONE_BYTE_STRING(isolate, "mode"), mode_string) .Check(); - obj->Set(context, - FIXED_ONE_BYTE_STRING(isolate, "clientId"), - String::NewFromUtf8(isolate, client_id.c_str()).ToLocalChecked()) + Local client_id_string; + if (!String::NewFromUtf8(isolate, client_id.c_str()) + .ToLocal(&client_id_string)) { + return Local(); + } + obj->Set( + context, FIXED_ONE_BYTE_STRING(isolate, "clientId"), client_id_string) .Check(); return obj; @@ -654,12 +685,21 @@ void CreatePerIsolateProperties(IsolateData* isolate_data, SetMethod(isolate, target, "request", LockManager::Request); SetMethod(isolate, target, "query", LockManager::Query); - target->Set(FIXED_ONE_BYTE_STRING(isolate, "LOCK_MODE_SHARED"), - String::NewFromUtf8(isolate, kSharedMode).ToLocalChecked()); - target->Set(FIXED_ONE_BYTE_STRING(isolate, "LOCK_MODE_EXCLUSIVE"), - String::NewFromUtf8(isolate, kExclusiveMode).ToLocalChecked()); - target->Set(FIXED_ONE_BYTE_STRING(isolate, "LOCK_STOLEN_ERROR"), - String::NewFromUtf8(isolate, kLockStolenError).ToLocalChecked()); + 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); + } } void CreatePerContextProperties(Local target, @@ -672,9 +712,7 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(LockManager::Query); } -} // namespace locks -} // namespace worker -} // namespace node +} // namespace node::worker::locks NODE_BINDING_CONTEXT_AWARE_INTERNAL( locks, node::worker::locks::CreatePerContextProperties) diff --git a/src/node_locks.h b/src/node_locks.h index fc7f7a2315b79d..f45fa85958e448 100644 --- a/src/node_locks.h +++ b/src/node_locks.h @@ -12,13 +12,11 @@ #include "node_mutex.h" #include "v8.h" -namespace node { -namespace worker { -namespace locks { +namespace node::worker::locks { class Lock final { public: - enum Mode { kShared, kExclusive }; + enum class Mode { Shared, Exclusive }; Lock(Environment* env, const std::u16string& name, @@ -56,6 +54,20 @@ class Lock final { v8::Global released_promise_; }; +class LockHolder final { + public: + explicit LockHolder(std::shared_ptr lock) : lock_(std::move(lock)) {} + ~LockHolder() = default; + + LockHolder(const LockHolder&) = delete; + LockHolder& operator=(const LockHolder&) = delete; + + std::shared_ptr lock() const { return lock_; } + + private: + std::shared_ptr lock_; +}; + class LockRequest final { public: LockRequest(Environment* env, @@ -134,9 +146,7 @@ class LockManager final { std::unordered_set registered_envs_; }; -} // namespace locks -} // namespace worker -} // namespace node +} // namespace node::worker::locks #endif // NODE_WANT_INTERNALS #endif // SRC_NODE_LOCKS_H_ diff --git a/test/parallel/test-web-locks.js b/test/parallel/test-web-locks.js index 1eb6777b1dab09..541efcf6b13e8c 100644 --- a/test/parallel/test-web-locks.js +++ b/test/parallel/test-web-locks.js @@ -1,4 +1,5 @@ 'use strict'; +// Flags: --expose-gc require('../common'); const { describe, it } = require('node:test'); @@ -139,4 +140,37 @@ describe('Web Locks with worker threads', () => { await worker.terminate(); }); + + it('should handle many concurrent locks without hanging', async () => { + if (global.gc) global.gc(); + const before = process.memoryUsage().rss; + + let callbackCount = 0; + let resolveCount = 0; + + const promises = []; + for (let i = 0; i < 100; i++) { + const promise = navigator.locks.request(`test-${i}`, async (lock) => { + callbackCount++; + const innerPromise = navigator.locks.request(`inner-${i}`, async () => { + resolveCount++; + return 'done'; + }); + await innerPromise; + return `completed-${lock.name}`; + }); + + promises.push(promise); + } + + await Promise.all(promises); + + if (global.gc) global.gc(); + + const after = process.memoryUsage().rss; + + assert.strictEqual(callbackCount, 100); + assert.strictEqual(resolveCount, 100); + assert(after < before * 3); + }); }); From de99211eef364a49a3ee03ac404ff7c8a38e4d6d Mon Sep 17 00:00:00 2001 From: ishabi Date: Tue, 17 Jun 2025 11:52:00 +0200 Subject: [PATCH 03/16] add lock and lockmanager to globals doc --- doc/api/globals.md | 1 + 1 file changed, 1 insertion(+) diff --git a/doc/api/globals.md b/doc/api/globals.md index 0de73e58e9414d..b65b034f072451 100644 --- a/doc/api/globals.md +++ b/doc/api/globals.md @@ -1313,6 +1313,7 @@ A browser-compatible implementation of [`WritableStreamDefaultWriter`][]. [`DecompressionStream`]: webstreams.md#class-decompressionstream [`EventTarget` and `Event` API]: events.md#eventtarget-and-event-api [`LockManager`]: worker_threads.md#class-lockmanager +[`Lock`]: worker_threads.md#class-lock [`MessageChannel`]: worker_threads.md#class-messagechannel [`MessagePort`]: worker_threads.md#class-messageport [`PerformanceEntry`]: perf_hooks.md#class-performanceentry From 89ed23468479a78bdfb25c14ecad277ab0dd2182 Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 18 Jun 2025 15:29:44 +0200 Subject: [PATCH 04/16] remove lock and lockmanager from globals --- doc/api/globals.md | 1 - .../bootstrap/web/exposed-window-or-worker.js | 3 --- test/wpt/status/web-locks.json | 25 +++++++++++++++++++ 3 files changed, 25 insertions(+), 4 deletions(-) diff --git a/doc/api/globals.md b/doc/api/globals.md index b65b034f072451..0de73e58e9414d 100644 --- a/doc/api/globals.md +++ b/doc/api/globals.md @@ -1313,7 +1313,6 @@ A browser-compatible implementation of [`WritableStreamDefaultWriter`][]. [`DecompressionStream`]: webstreams.md#class-decompressionstream [`EventTarget` and `Event` API]: events.md#eventtarget-and-event-api [`LockManager`]: worker_threads.md#class-lockmanager -[`Lock`]: worker_threads.md#class-lock [`MessageChannel`]: worker_threads.md#class-messagechannel [`MessagePort`]: worker_threads.md#class-messageport [`PerformanceEntry`]: perf_hooks.md#class-performanceentry diff --git a/lib/internal/bootstrap/web/exposed-window-or-worker.js b/lib/internal/bootstrap/web/exposed-window-or-worker.js index 46ed0c037ca5e3..f86efb261f0c9d 100644 --- a/lib/internal/bootstrap/web/exposed-window-or-worker.js +++ b/lib/internal/bootstrap/web/exposed-window-or-worker.js @@ -127,6 +127,3 @@ if (internalBinding('config').hasOpenSSL) { }, }, 'crypto') }); } - -// https://w3c.github.io/web-locks/ -exposeLazyInterfaces(globalThis, 'internal/locks', ['LockManager', 'Lock']); diff --git a/test/wpt/status/web-locks.json b/test/wpt/status/web-locks.json index 8a547c6c416959..5e5bf3ba197237 100644 --- a/test/wpt/status/web-locks.json +++ b/test/wpt/status/web-locks.json @@ -1,4 +1,29 @@ { + "idlharness.https.any.js": { + "fail": { + "expected": [ + "LockManager interface: existence and properties of interface object", + "LockManager interface object length", + "LockManager interface object name", + "LockManager interface: existence and properties of interface prototype object", + "LockManager interface: existence and properties of interface prototype object's \"constructor\" property", + "LockManager interface: existence and properties of interface prototype object's @@unscopables property", + "LockManager interface: operation request(DOMString, LockGrantedCallback)", + "LockManager interface: operation request(DOMString, LockOptions, LockGrantedCallback)", + "LockManager interface: operation query()", + "LockManager must be primary interface of navigator.locks", + "Lock interface: existence and properties of interface object", + "Lock interface object length", + "Lock interface object name", + "Lock interface: existence and properties of interface prototype object", + "Lock interface: existence and properties of interface prototype object's \"constructor\" property", + "Lock interface: existence and properties of interface prototype object's @@unscopables property", + "Lock interface: attribute name", + "Lock interface: attribute mode", + "Lock must be primary interface of lock" + ] + } + }, "non-secure-context.any.js": { "skip": "navigator.locks is only present in secure contexts" }, From eb859d167b0696c7d91299ab5b12957b46fb2714 Mon Sep 17 00:00:00 2001 From: ishabi Date: Fri, 20 Jun 2025 15:58:21 +0200 Subject: [PATCH 05/16] attemp to atach catch on c++ by defering to next microtask --- lib/internal/locks.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/internal/locks.js b/lib/internal/locks.js index 674e2d3cd55105..723db25ee5acdc 100644 --- a/lib/internal/locks.js +++ b/lib/internal/locks.js @@ -239,7 +239,9 @@ class LockManager { // When ifAvailable: true and lock is not available, C++ passes null to indicate no lock granted const wrapCallback = (internalLock) => { const lock = createLock(internalLock); - return callback(lock); + return PromiseResolve().then(() => { + return callback(lock); + }); }; // Standard request without signal From 44d090974e9f96a3c04135fcceac7a47177a1fac Mon Sep 17 00:00:00 2001 From: ishabi Date: Sat, 21 Jun 2025 11:07:20 +0200 Subject: [PATCH 06/16] separate promise fulfillment and rejection handlers --- lib/internal/locks.js | 12 +++---- src/node_locks.cc | 77 ++++++++++++++++++++++++++----------------- src/node_locks.h | 3 +- 3 files changed, 54 insertions(+), 38 deletions(-) diff --git a/lib/internal/locks.js b/lib/internal/locks.js index 723db25ee5acdc..4610bddaf8ea20 100644 --- a/lib/internal/locks.js +++ b/lib/internal/locks.js @@ -152,6 +152,10 @@ class LockManager { validateAbortSignal(signal, 'options.signal'); + if (signal) { + signal.throwIfAborted(); + } + if (name.startsWith('-')) { // If name starts with U+002D HYPHEN-MINUS (-), then reject promise with a // "NotSupportedError" DOMException. @@ -184,10 +188,6 @@ class LockManager { 'NotSupportedError'); } - if (signal) { - signal.throwIfAborted(); - } - const clientId = `node-${process.pid}-${threadId}`; // Handle requests with AbortSignal @@ -239,9 +239,7 @@ class LockManager { // When ifAvailable: true and lock is not available, C++ passes null to indicate no lock granted const wrapCallback = (internalLock) => { const lock = createLock(internalLock); - return PromiseResolve().then(() => { - return callback(lock); - }); + return callback(lock); }; // Standard request without signal diff --git a/src/node_locks.cc b/src/node_locks.cc index d35d06947d82df..0a5ac3ef897bc6 100644 --- a/src/node_locks.cc +++ b/src/node_locks.cc @@ -46,10 +46,7 @@ Lock::Lock(Environment* env, released_promise_.Reset(env_->isolate(), released); } -Lock::~Lock() { - waiting_promise_.Reset(); - released_promise_.Reset(); -} +Lock::~Lock() {} LockRequest::LockRequest(Environment* env, Local waiting, @@ -71,11 +68,7 @@ LockRequest::LockRequest(Environment* env, callback_.Reset(env_->isolate(), callback); } -LockRequest::~LockRequest() { - waiting_promise_.Reset(); - released_promise_.Reset(); - callback_.Reset(); -} +LockRequest::~LockRequest() {} bool LockManager::IsGrantable(const LockRequest* request) const { // Steal requests bypass all normal granting rules @@ -96,8 +89,8 @@ bool LockManager::IsGrantable(const LockRequest* request) const { return true; } -// Called when the user callback settles -static void OnLockCallbackSettled( +// Called when the user callback promise fulfills +static void OnLockCallbackFulfilled( const v8::FunctionCallbackInfo& info) { HandleScope handle_scope(info.GetIsolate()); Environment* env = Environment::GetCurrent(info); @@ -109,7 +102,23 @@ static void OnLockCallbackSettled( delete lock_holder; // Release the lock and continue processing the queue. - LockManager::GetCurrent()->ReleaseLockAndProcessQueue(env, lock, info[0]); + LockManager::GetCurrent()->ReleaseLockAndProcessQueue( + env, lock, info[0], false); +} + +// Called when the user callback promise rejects +static void OnLockCallbackRejected( + const v8::FunctionCallbackInfo& info) { + HandleScope handle_scope(info.GetIsolate()); + Environment* env = Environment::GetCurrent(info); + + auto* lock_holder = + static_cast(info.Data().As()->Value()); + std::shared_ptr lock = lock_holder->lock(); + delete lock_holder; + + LockManager::GetCurrent()->ReleaseLockAndProcessQueue( + env, lock, info[0], true); } void LockManager::CleanupStolenLocks(Environment* env) { @@ -351,14 +360,21 @@ void LockManager::ProcessQueue(Environment* env) { // Allocate a LockHolder on the heap to safely manage the lock's lifetime // until the user's callback promise settles. - // The LockHolder will be deleted in OnLockCallbackSettled. - auto* lock_holder = new LockHolder(granted_lock); - Local on_settled_callback; + auto* fulfill_holder = new LockHolder(granted_lock); + auto* reject_holder = new LockHolder(granted_lock); + Local on_fulfilled_callback; + Local on_rejected_callback; + if (!Function::New(context, - OnLockCallbackSettled, - External::New(isolate, lock_holder)) - .ToLocal(&on_settled_callback)) { - delete lock_holder; + OnLockCallbackFulfilled, + External::New(isolate, fulfill_holder)) + .ToLocal(&on_fulfilled_callback) || + !Function::New(context, + OnLockCallbackRejected, + External::New(isolate, reject_holder)) + .ToLocal(&on_rejected_callback)) { + delete fulfill_holder; + delete reject_holder; return; } @@ -373,7 +389,8 @@ void LockManager::ProcessQueue(Environment* env) { grantable_request->released_promise() ->Reject(context, rejection_value) .Check(); - delete lock_holder; + delete fulfill_holder; + delete reject_holder; { Mutex::ScopedLock scoped_lock(mutex_); ReleaseLock(granted_lock.get()); @@ -384,15 +401,17 @@ void LockManager::ProcessQueue(Environment* env) { grantable_request->waiting_promise() ->Resolve(context, callback_result) .Check(); - USE(promise->Then(context, on_settled_callback, on_settled_callback)); + USE(promise->Then( + context, on_fulfilled_callback, on_rejected_callback)); } } else { grantable_request->waiting_promise() ->Resolve(context, callback_result) .Check(); Local promise_args[] = {callback_result}; - USE(on_settled_callback->Call( + USE(on_fulfilled_callback->Call( context, Undefined(isolate), 1, promise_args)); + delete reject_holder; } } } @@ -541,7 +560,8 @@ void LockManager::Query(const FunctionCallbackInfo& args) { // Runs after the user callback (or its returned promise) settles. void LockManager::ReleaseLockAndProcessQueue(Environment* env, std::shared_ptr lock, - Local callback_result) { + Local callback_result, + bool was_rejected) { { Mutex::ScopedLock scoped_lock(mutex_); ReleaseLock(lock.get()); @@ -552,14 +572,11 @@ void LockManager::ReleaseLockAndProcessQueue(Environment* env, // For stolen locks, the released_promise was already rejected when marked as // stolen. if (!lock->is_stolen()) { - if (callback_result->IsPromise()) { - Local promise = callback_result.As(); - if (promise->State() == Promise::kFulfilled) { - lock->released_promise()->Resolve(context, promise->Result()).Check(); - } else { - lock->released_promise()->Reject(context, promise->Result()).Check(); - } + if (was_rejected) { + // The callback promise was rejected, so reject the final promise + lock->released_promise()->Reject(context, callback_result).Check(); } else { + // The callback promise was fulfilled, so resolve the final promise lock->released_promise()->Resolve(context, callback_result).Check(); } } diff --git a/src/node_locks.h b/src/node_locks.h index f45fa85958e448..e047a51e034e02 100644 --- a/src/node_locks.h +++ b/src/node_locks.h @@ -123,7 +123,8 @@ class LockManager final { static LockManager* GetCurrent() { return ¤t_; } void ReleaseLockAndProcessQueue(Environment* env, std::shared_ptr lock, - v8::Local result); + v8::Local result, + bool was_rejected = false); private: LockManager() = default; From 49e2bba028c7ebab2df00eb9d400b1ae43627097 Mon Sep 17 00:00:00 2001 From: ishabi Date: Sun, 22 Jun 2025 00:06:08 +0200 Subject: [PATCH 07/16] add cjs/esm code in locks doc --- doc/api/worker_threads.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md index 289a454d8b1906..6acce974555880 100644 --- a/doc/api/worker_threads.md +++ b/doc/api/worker_threads.md @@ -806,7 +806,17 @@ added: REPLACEME --> The `LockManager` interface provides methods for requesting and introspecting -locks. To obtain a `LockManager` instance use `require('node:worker_threads').locks`. +locks. To obtain a `LockManager` instance use + +```mjs +import { locks } from 'node:worker_threads'; +``` + +```cjs +'use strict'; + +const { locks } = require('node:worker_threads'); +``` This implementation matches the [browser `LockManager`][] API. From 8a9d4a72af059b1837729d6e8278b289ebf83136 Mon Sep 17 00:00:00 2001 From: ishabi Date: Sun, 22 Jun 2025 20:31:50 +0200 Subject: [PATCH 08/16] add more tests and doc --- src/node_locks.cc | 7 ++--- src/node_locks.h | 14 +++++++-- test/parallel/test-web-locks.js | 50 +++++++++++++++++++++++++++++++-- 3 files changed, 63 insertions(+), 8 deletions(-) diff --git a/src/node_locks.cc b/src/node_locks.cc index 0a5ac3ef897bc6..064140903f8311 100644 --- a/src/node_locks.cc +++ b/src/node_locks.cc @@ -46,8 +46,6 @@ Lock::Lock(Environment* env, released_promise_.Reset(env_->isolate(), released); } -Lock::~Lock() {} - LockRequest::LockRequest(Environment* env, Local waiting, Local released, @@ -68,8 +66,6 @@ LockRequest::LockRequest(Environment* env, callback_.Reset(env_->isolate(), callback); } -LockRequest::~LockRequest() {} - bool LockManager::IsGrantable(const LockRequest* request) const { // Steal requests bypass all normal granting rules if (request->steal()) return true; @@ -618,6 +614,7 @@ void LockManager::WakeEnvironment(Environment* target_env) { void LockManager::CleanupEnvironment(Environment* env_to_cleanup) { Mutex::ScopedLock scoped_lock(mutex_); + // Remove every held lock that belongs to this Environment. for (auto resource_iter = held_locks_.begin(); resource_iter != held_locks_.end();) { auto& resource_locks = resource_iter->second; @@ -636,6 +633,7 @@ void LockManager::CleanupEnvironment(Environment* env_to_cleanup) { } } + // Remove every pending request submitted by this Environment. for (auto request_iter = pending_queue_.begin(); request_iter != pending_queue_.end();) { if ((*request_iter)->env() == env_to_cleanup) { @@ -645,6 +643,7 @@ void LockManager::CleanupEnvironment(Environment* env_to_cleanup) { } } + // Finally, remove it from registered_envs_ registered_envs_.erase(env_to_cleanup); } diff --git a/src/node_locks.h b/src/node_locks.h index e047a51e034e02..5291baad8f15b5 100644 --- a/src/node_locks.h +++ b/src/node_locks.h @@ -24,22 +24,30 @@ class Lock final { const std::string& client_id, v8::Local waiting, v8::Local released); - ~Lock(); + ~Lock() = default; Lock(const Lock&) = delete; Lock& operator=(const Lock&) = delete; + // Resource name for this lock as DOMString const std::u16string& name() const { return name_; } + // Lock mode (shared or exclusive). Mode mode() const { return mode_; } + // Client identifier string. const std::string& client_id() const { return client_id_; } + // Environment that owns this lock. Environment* env() const { return env_; } + // Returns true if this lock was stolen by another request. bool is_stolen() const { return stolen_; } + // Marks this lock as stolen. void mark_stolen() { stolen_ = true; } + // Promise that resolves when the user callback completes. v8::Local waiting_promise() { return waiting_promise_.Get(env_->isolate()); } + // Promise that resolves when the lock is finally released. v8::Local released_promise() { return released_promise_.Get(env_->isolate()); } @@ -79,7 +87,7 @@ class LockRequest final { const std::string& client_id, bool steal, bool if_available); - ~LockRequest(); + ~LockRequest() = default; LockRequest(const LockRequest&) = delete; LockRequest& operator=(const LockRequest&) = delete; @@ -88,6 +96,7 @@ class LockRequest final { Lock::Mode mode() const { return mode_; } const std::string& client_id() const { return client_id_; } bool steal() const { return steal_; } + // Returns true if this is an ifAvailable request. bool if_available() const { return if_available_; } Environment* env() const { return env_; } @@ -141,6 +150,7 @@ class LockManager final { static LockManager current_; mutable Mutex mutex_; + // All entries for a given Environment* are purged in CleanupEnvironment(). std::unordered_map>> held_locks_; std::deque> pending_queue_; diff --git a/test/parallel/test-web-locks.js b/test/parallel/test-web-locks.js index 541efcf6b13e8c..4fe16392b8fe74 100644 --- a/test/parallel/test-web-locks.js +++ b/test/parallel/test-web-locks.js @@ -1,10 +1,11 @@ 'use strict'; // Flags: --expose-gc -require('../common'); +const common = require('../common'); const { describe, it } = require('node:test'); const assert = require('node:assert'); -const { Worker } = require('worker_threads'); +const { Worker } = require('node:worker_threads'); +const { AsyncLocalStorage } = require('node:async_hooks'); describe('Web Locks with worker threads', () => { it('should handle exclusive locks', async () => { @@ -173,4 +174,49 @@ describe('Web Locks with worker threads', () => { assert.strictEqual(resolveCount, 100); assert(after < before * 3); }); + + it('should preserve AsyncLocalStorage context across lock callback', async () => { + const als = new AsyncLocalStorage(); + const store = { id: 'lock' }; + + als.run(store, () => { + navigator.locks + .request('als-context-test', async () => { + assert.strictEqual(als.getStore(), store); + }) + .catch(common.mustNotCall()); + }); + }); + + it('should clean up when worker is terminated with a pending lock', async () => { + // Acquire the lock in the main thread so that the worker's request will be pending + await navigator.locks.request('cleanup-test', async () => { + // Launch a worker that requests the same lock + const worker = new Worker(` + const { parentPort } = require('worker_threads'); + + parentPort.postMessage({ requesting: true }); + + navigator.locks.request('cleanup-test', async () => { + return 'should-not-complete'; + }).catch(err => { + parentPort.postMessage({ error: err.name }); + }); + `, { eval: true }); + + const requestSignal = await new Promise((resolve) => { + worker.once('message', resolve); + }); + + assert.strictEqual(requestSignal.requesting, true); + + await worker.terminate(); + + }); + + // Request the lock again to make sure cleanup succeeded + await navigator.locks.request('cleanup-test', async (lock) => { + assert.strictEqual(lock.name, 'cleanup-test'); + }); + }); }); From d11a46b4413554c5254bac5501ca61075cad1e76 Mon Sep 17 00:00:00 2001 From: ishabi Date: Mon, 23 Jun 2025 17:59:46 +0200 Subject: [PATCH 09/16] update bootstrap modules --- test/parallel/test-bootstrap-modules.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/parallel/test-bootstrap-modules.js b/test/parallel/test-bootstrap-modules.js index deffc73ffb6993..a66a3ec9ce360f 100644 --- a/test/parallel/test-bootstrap-modules.js +++ b/test/parallel/test-bootstrap-modules.js @@ -126,9 +126,11 @@ if (isMainThread) { ].forEach(expected.beforePreExec.add.bind(expected.beforePreExec)); } else { // Worker. [ + 'Internal Binding locks', 'NativeModule diagnostics_channel', 'NativeModule internal/abort_controller', 'NativeModule internal/error_serdes', + 'NativeModule internal/locks', 'NativeModule internal/perf/event_loop_utilization', 'NativeModule internal/process/worker_thread_only', 'NativeModule internal/streams/add-abort-signal', @@ -149,7 +151,6 @@ if (isMainThread) { 'NativeModule internal/streams/writable', 'NativeModule internal/worker', 'NativeModule internal/worker/io', - 'NativeModule internal/worker/locks', 'NativeModule internal/worker/messaging', 'NativeModule stream', 'NativeModule stream/promises', From 44007f375a384fbeef9900690976055e1f8dfbed Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 26 Jun 2025 00:44:24 +0200 Subject: [PATCH 10/16] fix race condition in wpt and pr comments --- src/node_locks.cc | 403 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 283 insertions(+), 120 deletions(-) diff --git a/src/node_locks.cc b/src/node_locks.cc index 064140903f8311..680e9910ecc277 100644 --- a/src/node_locks.cc +++ b/src/node_locks.cc @@ -19,6 +19,7 @@ using v8::FunctionCallbackInfo; using v8::HandleScope; using v8::Isolate; using v8::Local; +using v8::MaybeLocal; using v8::Object; using v8::ObjectTemplate; using v8::Promise; @@ -29,11 +30,11 @@ static constexpr const char* kSharedMode = "shared"; static constexpr const char* kExclusiveMode = "exclusive"; static constexpr const char* kLockStolenError = "LOCK_STOLEN"; -static Local CreateLockInfoObject(Isolate* isolate, - Local context, - const std::u16string& name, - Lock::Mode mode, - const std::string& client_id); +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, @@ -117,10 +118,34 @@ static void OnLockCallbackRejected( env, lock, info[0], true); } +// Called when the promise returned from the user's callback resolves +static void OnIfAvailableFulfill( + const v8::FunctionCallbackInfo& info) { + HandleScope handle_scope(info.GetIsolate()); + auto* holder = static_cast*>( + info.Data().As()->Value()); + USE(holder->Get(info.GetIsolate()) + ->Resolve(info.GetIsolate()->GetCurrentContext(), info[0])); + + delete holder; +} + +// Called when the promise returned from the user's callback rejects +static void OnIfAvailableReject( + const v8::FunctionCallbackInfo& info) { + HandleScope handle_scope(info.GetIsolate()); + auto* holder = static_cast*>( + info.Data().As()->Value()); + USE(holder->Get(info.GetIsolate()) + ->Reject(info.GetIsolate()->GetCurrentContext(), info[0])); + + delete holder; +} + void LockManager::CleanupStolenLocks(Environment* env) { std::vector resources_to_clean; - // Collect resources to clean + // Iterate held locks and remove entries that were stolen from other envs. { Mutex::ScopedLock scoped_lock(mutex_); @@ -186,6 +211,20 @@ void LockManager::ProcessQueue(Environment* env) { std::unique_ptr if_available_request; std::unordered_set other_envs_to_wake; + /** + * First pass over pending_queue_ + * 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 + * – For our Environment, pick one of: + * * grantable_request – can be granted now + * * if_available_request – user asked for ifAvailable and the + * resource is currently busy + * * otherwise we skip and keep scanning + */ + { std::unordered_map first_seen_for_resource; @@ -198,7 +237,6 @@ void LockManager::ProcessQueue(Environment* env) { // Collect unique environments to wake up later if (request->env() != env) { other_envs_to_wake.insert(request->env()); - continue; } // During a single pass, the first time we see a resource name is the @@ -211,7 +249,25 @@ void LockManager::ProcessQueue(Environment* env) { bool has_earlier_request_for_same_resource = (first_for_resource != request); - if (has_earlier_request_for_same_resource || !IsGrantable(request)) { + bool should_wait_for_earlier_requests = false; + + if (has_earlier_request_for_same_resource) { + // Check if this request is compatible with the earliest pending + // request first_for_resource + if (request->mode() == Lock::Mode::Exclusive || + first_for_resource->mode() == Lock::Mode::Exclusive) { + // Exclusive locks are incompatible with everything + should_wait_for_earlier_requests = true; + } + // If both are shared, they're compatible and can proceed + } + + // Only process requests from the current environment + if (request->env() != env) { + continue; + } + + if (should_wait_for_earlier_requests || !IsGrantable(request)) { if (request->if_available()) { // ifAvailable request when resource not available: grant with null if_available_request = std::move(*queue_iter); @@ -234,10 +290,12 @@ void LockManager::ProcessQueue(Environment* env) { } /** - * ifAvailable: - * Grant the lock only if it is immediately available; - * otherwise invoke the callback with null and resolve the promises. - * Check wrapCallback function in locks.js + * 1- We call the user callback immediately with `null` to signal + * that the lock was not granted - Check wrapCallback function in + * locks.js 2- Depending on what the callback returns we settle the two + * internal promises + * 3- No lock is added to held_locks_ in this path, so nothing to + * remove later */ if (if_available_request) { Local null_arg = Null(isolate); @@ -247,27 +305,75 @@ void LockManager::ProcessQueue(Environment* env) { if (!if_available_request->callback() ->Call(context, Undefined(isolate), 1, &null_arg) .ToLocal(&callback_result)) { - if_available_request->waiting_promise() - ->Reject(context, try_catch_scope.Exception()) - .Check(); - if_available_request->released_promise() - ->Reject(context, try_catch_scope.Exception()) - .Check(); + USE(if_available_request->waiting_promise()->Reject( + context, try_catch_scope.Exception())); + USE(if_available_request->released_promise()->Reject( + context, try_catch_scope.Exception())); return; } } - if_available_request->waiting_promise() - ->Resolve(context, callback_result) - .Check(); - if_available_request->released_promise() - ->Resolve(context, callback_result) - .Check(); + if (callback_result->IsPromise()) { + Local p = callback_result.As(); + + // Use a Global holder so the resolver survives until the promise + // settles. + auto* fulf_holder = new v8::Global( + isolate, if_available_request->released_promise()); + auto* rej_holder = new v8::Global( + isolate, if_available_request->released_promise()); + + Local on_fulfilled; + Local on_rejected; + CHECK(Function::New(context, + OnIfAvailableFulfill, + External::New(isolate, fulf_holder)) + .ToLocal(&on_fulfilled)); + CHECK(Function::New(context, + OnIfAvailableReject, + External::New(isolate, rej_holder)) + .ToLocal(&on_rejected)); + + if (p->Then(context, on_fulfilled, on_rejected).IsEmpty()) { + // Attaching handlers failed; reject promises and return. + Local err_str_local = + String::NewFromUtf8(isolate, "Failed to attach promise handlers") + .ToLocalChecked(); + Local err_val = Exception::Error(err_str_local); + + USE(if_available_request->waiting_promise()->Reject(context, + err_val)); + USE(if_available_request->released_promise()->Reject(context, + err_val)); + + return; + } + + // After handlers are attached, resolve waiting_promise with the + // promise. + USE(if_available_request->waiting_promise()->Resolve(context, p)); + + return; + } + + // Non-promise callback result: settle both promises right away. + USE(if_available_request->waiting_promise()->Resolve(context, + callback_result)); + USE(if_available_request->released_promise()->Resolve(context, + callback_result)); + return; } if (!grantable_request) return; - // Handle steal operations with minimal mutex scope + /** + * 1- We grant the lock immediately even if other envs hold it + * 2- All existing locks with the same name are marked stolen, their + * released_promise is rejected, and their owners are woken so they + * can observe the rejection + * 3- We remove stolen locks that belong to this env right away; other + * envs will clean up in their next queue pass + */ if (grantable_request->steal()) { std::unordered_set envs_to_notify; @@ -287,7 +393,8 @@ void LockManager::ProcessQueue(Environment* env) { return; } Local error = Exception::Error(error_string); - existing_lock->released_promise()->Reject(context, error).Check(); + + USE(existing_lock->released_promise()->Reject(context, error)); } // Remove stolen locks from current environment immediately @@ -327,16 +434,18 @@ void LockManager::ProcessQueue(Environment* env) { held_locks_[grantable_request->name()].push_back(granted_lock); } - // Call user callback - Local lock_info_obj = - CreateLockInfoObject(isolate, - context, - grantable_request->name(), - grantable_request->mode(), - grantable_request->client_id()); - if (lock_info_obj.IsEmpty()) { + // Create and store the new granted lock + Local lock_info_obj; + if (!CreateLockInfoObject(isolate, + context, + grantable_request->name(), + grantable_request->mode(), + grantable_request->client_id()) + .ToLocal(&lock_info_obj)) { return; } + + // Call user callback Local callback_arg = lock_info_obj; Local callback_result; { @@ -344,12 +453,11 @@ void LockManager::ProcessQueue(Environment* env) { if (!grantable_request->callback() ->Call(context, Undefined(isolate), 1, &callback_arg) .ToLocal(&callback_result)) { - grantable_request->waiting_promise() - ->Reject(context, try_catch_scope.Exception()) - .Check(); - grantable_request->released_promise() - ->Reject(context, try_catch_scope.Exception()) - .Check(); + USE(grantable_request->waiting_promise()->Reject( + context, try_catch_scope.Exception())); + USE(grantable_request->released_promise()->Reject( + context, try_catch_scope.Exception())); + return; } } @@ -361,15 +469,22 @@ void LockManager::ProcessQueue(Environment* env) { Local on_fulfilled_callback; Local on_rejected_callback; + // Create fulfilled callback first if (!Function::New(context, OnLockCallbackFulfilled, External::New(isolate, fulfill_holder)) - .ToLocal(&on_fulfilled_callback) || - !Function::New(context, + .ToLocal(&on_fulfilled_callback)) { + delete fulfill_holder; + delete reject_holder; + return; + } + + // Create rejected callback second + if (!Function::New(context, OnLockCallbackRejected, External::New(isolate, reject_holder)) .ToLocal(&on_rejected_callback)) { - delete fulfill_holder; + // fulfill_holder is now owned by the External, don't delete it delete reject_holder; return; } @@ -377,36 +492,37 @@ void LockManager::ProcessQueue(Environment* env) { // Handle promise chain if (callback_result->IsPromise()) { Local promise = callback_result.As(); - if (promise->State() == Promise::kRejected) { - Local rejection_value = promise->Result(); - grantable_request->waiting_promise() - ->Reject(context, rejection_value) - .Check(); - grantable_request->released_promise() - ->Reject(context, rejection_value) - .Check(); - delete fulfill_holder; - delete reject_holder; - { - Mutex::ScopedLock scoped_lock(mutex_); - ReleaseLock(granted_lock.get()); - } - ProcessQueue(env); + if (promise->Then(context, on_fulfilled_callback, on_rejected_callback) + .IsEmpty()) { + // Then() failed, reject both promises and return + Local err_str_local = + String::NewFromUtf8(isolate, "Failed to attach promise handlers") + .ToLocalChecked(); + Local err_val = Exception::Error(err_str_local); + + USE(grantable_request->waiting_promise()->Reject(context, err_val)); + USE(grantable_request->released_promise()->Reject(context, err_val)); + return; - } else { - grantable_request->waiting_promise() - ->Resolve(context, callback_result) - .Check(); - USE(promise->Then( - context, on_fulfilled_callback, on_rejected_callback)); } + + // Lock granted: waiting_promise resolves now with the promise returned + // by the callback; on_fulfilled/on_rejected will release the lock when + // that promise settles. + USE(grantable_request->waiting_promise()->Resolve(context, + callback_result)); } else { - grantable_request->waiting_promise() - ->Resolve(context, callback_result) - .Check(); + USE(grantable_request->waiting_promise()->Resolve(context, + callback_result)); Local promise_args[] = {callback_result}; - USE(on_fulfilled_callback->Call( - context, Undefined(isolate), 1, promise_args)); + if (on_fulfilled_callback + ->Call(context, Undefined(isolate), 1, promise_args) + .IsEmpty()) { + // Callback threw an error, handle it like a rejected promise + // The error is already propagated through the TryCatch in the + // callback + return; + } delete reject_holder; } } @@ -427,12 +543,12 @@ void LockManager::Request(const FunctionCallbackInfo& args) { Local context = env->context(); CHECK_EQ(args.Length(), 6); - CHECK(args[0]->IsString()); - CHECK(args[1]->IsString()); - CHECK(args[2]->IsString()); - CHECK(args[3]->IsBoolean()); - CHECK(args[4]->IsBoolean()); - CHECK(args[5]->IsFunction()); + CHECK(args[0]->IsString()); // name + CHECK(args[1]->IsString()); // clientId + CHECK(args[2]->IsString()); // mode + CHECK(args[3]->IsBoolean()); // steal + 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); @@ -451,15 +567,17 @@ void LockManager::Request(const FunctionCallbackInfo& args) { mode_str == kSharedMode ? Lock::Mode::Shared : Lock::Mode::Exclusive; Local waiting_promise; - if (!Promise::Resolver::New(context).ToLocal(&waiting_promise)) { - return; - } Local released_promise; - if (!Promise::Resolver::New(context).ToLocal(&released_promise)) { + + if (!Promise::Resolver::New(context).ToLocal(&waiting_promise) || + !Promise::Resolver::New(context).ToLocal(&released_promise)) { return; } - args.GetReturnValue().Set(released_promise->GetPromise()); + // Mark both internal promises as handled to prevent unhandled rejection + // warnings + waiting_promise->GetPromise()->MarkAsHandled(); + released_promise->GetPromise()->MarkAsHandled(); LockManager* manager = GetCurrent(); { @@ -488,6 +606,8 @@ void LockManager::Request(const FunctionCallbackInfo& args) { } manager->ProcessQueue(env); + + args.GetReturnValue().Set(released_promise->GetPromise()); } void LockManager::Query(const FunctionCallbackInfo& args) { @@ -500,6 +620,8 @@ void LockManager::Query(const FunctionCallbackInfo& args) { if (!Promise::Resolver::New(context).ToLocal(&resolver)) { return; } + + // Always set the return value first so Javascript gets a promise args.GetReturnValue().Set(resolver->GetPromise()); Local result = Object::New(isolate); @@ -511,19 +633,30 @@ void LockManager::Query(const FunctionCallbackInfo& args) { 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) { - Local lock_info = - CreateLockInfoObject(isolate, - context, - held_lock->name(), - held_lock->mode(), - held_lock->client_id()); - if (lock_info.IsEmpty()) { + if (!CreateLockInfoObject(isolate, + context, + held_lock->name(), + held_lock->mode(), + held_lock->client_id()) + .ToLocal(&lock_info)) { + Local error_msg = + String::NewFromUtf8(isolate, + "Failed to create lock info object") + .ToLocalChecked(); + resolver->Reject(context, Exception::Error(error_msg)).Check(); + return; + } + if (held_list->Set(context, index++, lock_info).IsNothing()) { + Local error_msg = + String::NewFromUtf8(isolate, "Failed to build held locks array") + .ToLocalChecked(); + resolver->Reject(context, Exception::Error(error_msg)).Check(); return; } - held_list->Set(context, index++, lock_info).Check(); } } } @@ -531,26 +664,44 @@ void LockManager::Query(const FunctionCallbackInfo& args) { index = 0; for (const auto& pending_request : manager->pending_queue_) { if (pending_request->env() == env) { - Local lock_info = - CreateLockInfoObject(isolate, - context, - pending_request->name(), - pending_request->mode(), - pending_request->client_id()); - if (lock_info.IsEmpty()) { + if (!CreateLockInfoObject(isolate, + context, + pending_request->name(), + pending_request->mode(), + pending_request->client_id()) + .ToLocal(&lock_info)) { + Local error_msg = + String::NewFromUtf8(isolate, "Failed to create lock info object") + .ToLocalChecked(); + resolver->Reject(context, Exception::Error(error_msg)).Check(); + return; + } + if (pending_list->Set(context, index++, lock_info).IsNothing()) { + Local error_msg = + String::NewFromUtf8(isolate, + "Failed to build pending locks array") + .ToLocalChecked(); + resolver->Reject(context, Exception::Error(error_msg)).Check(); return; } - pending_list->Set(context, index++, lock_info).Check(); } } } - result->Set(context, FIXED_ONE_BYTE_STRING(isolate, "held"), held_list) - .Check(); - result->Set(context, FIXED_ONE_BYTE_STRING(isolate, "pending"), pending_list) - .Check(); + if (result->Set(context, FIXED_ONE_BYTE_STRING(isolate, "held"), held_list) + .IsNothing() || + result + ->Set( + context, FIXED_ONE_BYTE_STRING(isolate, "pending"), pending_list) + .IsNothing()) { + Local error_msg = + String::NewFromUtf8(isolate, "Failed to build query result object") + .ToLocalChecked(); + resolver->Reject(context, Exception::Error(error_msg)).Check(); + return; + } - resolver->Resolve(context, result).Check(); + USE(resolver->Resolve(context, result)); } // Runs after the user callback (or its returned promise) settles. @@ -569,11 +720,11 @@ void LockManager::ReleaseLockAndProcessQueue(Environment* env, // stolen. if (!lock->is_stolen()) { if (was_rejected) { - // The callback promise was rejected, so reject the final promise - lock->released_promise()->Reject(context, callback_result).Check(); + // Propagate rejection from the user callback + USE(lock->released_promise()->Reject(context, callback_result)); } else { - // The callback promise was fulfilled, so resolve the final promise - lock->released_promise()->Resolve(context, callback_result).Check(); + // Propagate fulfilment + USE(lock->released_promise()->Resolve(context, callback_result)); } } @@ -602,6 +753,7 @@ void LockManager::ReleaseLock(Lock* lock) { void LockManager::WakeEnvironment(Environment* target_env) { if (target_env == nullptr || target_env->is_stopping()) return; + // 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()) { LockManager::GetCurrent()->ProcessQueue(env_to_wake); @@ -653,11 +805,11 @@ void LockManager::OnEnvironmentCleanup(void* arg) { LockManager::GetCurrent()->CleanupEnvironment(env); } -static Local CreateLockInfoObject(Isolate* isolate, - Local context, - const std::u16string& name, - Lock::Mode mode, - const std::string& client_id) { +static MaybeLocal CreateLockInfoObject(Isolate* isolate, + Local context, + const std::u16string& name, + Lock::Mode mode, + const std::string& client_id) { Local obj = Object::New(isolate); Local name_string; @@ -666,29 +818,36 @@ static Local CreateLockInfoObject(Isolate* isolate, v8::NewStringType::kNormal, static_cast(name.length())) .ToLocal(&name_string)) { - return Local(); + return MaybeLocal(); + } + if (obj->Set(context, FIXED_ONE_BYTE_STRING(isolate, "name"), name_string) + .IsNothing()) { + return MaybeLocal(); } - obj->Set(context, FIXED_ONE_BYTE_STRING(isolate, "name"), name_string) - .Check(); Local mode_string; if (!String::NewFromUtf8( isolate, mode == Lock::Mode::Exclusive ? kExclusiveMode : kSharedMode) .ToLocal(&mode_string)) { - return Local(); + return MaybeLocal(); + } + if (obj->Set(context, FIXED_ONE_BYTE_STRING(isolate, "mode"), mode_string) + .IsNothing()) { + return MaybeLocal(); } - obj->Set(context, FIXED_ONE_BYTE_STRING(isolate, "mode"), mode_string) - .Check(); Local client_id_string; if (!String::NewFromUtf8(isolate, client_id.c_str()) .ToLocal(&client_id_string)) { - return Local(); + return MaybeLocal(); + } + if (obj->Set(context, + FIXED_ONE_BYTE_STRING(isolate, "clientId"), + client_id_string) + .IsNothing()) { + return MaybeLocal(); } - obj->Set( - context, FIXED_ONE_BYTE_STRING(isolate, "clientId"), client_id_string) - .Check(); return obj; } @@ -726,6 +885,10 @@ void CreatePerContextProperties(Local target, void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(LockManager::Request); registry->Register(LockManager::Query); + registry->Register(OnLockCallbackFulfilled); + registry->Register(OnLockCallbackRejected); + registry->Register(OnIfAvailableFulfill); + registry->Register(OnIfAvailableReject); } } // namespace node::worker::locks From ecdad5d97715fec3dc5c82291b985b3bf7c4a034 Mon Sep 17 00:00:00 2001 From: ishabi Date: Thu, 26 Jun 2025 12:24:57 +0200 Subject: [PATCH 11/16] mark held wpt test as flaky --- src/env_properties.h | 4 +++ src/node_locks.cc | 51 ++++++++++------------------------ test/wpt/status/web-locks.json | 8 ++++++ 3 files changed, 27 insertions(+), 36 deletions(-) diff --git a/src/env_properties.h b/src/env_properties.h index 70f3da751eb341..86a3e7b8420070 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -98,6 +98,7 @@ V(changes_string, "changes") \ V(channel_string, "channel") \ V(chunks_sent_since_last_write_string, "chunksSentSinceLastWrite") \ + V(clientId_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 " \ @@ -193,6 +194,7 @@ 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") \ @@ -254,6 +256,7 @@ V(messageerror_string, "messageerror") \ V(mgf1_hash_algorithm_string, "mgf1HashAlgorithm") \ V(minttl_string, "minttl") \ + V(mode_string, "mode") \ V(module_string, "module") \ V(modulus_string, "modulus") \ V(modulus_length_string, "modulusLength") \ @@ -300,6 +303,7 @@ 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") \ diff --git a/src/node_locks.cc b/src/node_locks.cc index 680e9910ecc277..e956b7b7409faa 100644 --- a/src/node_locks.cc +++ b/src/node_locks.cc @@ -643,18 +643,12 @@ void LockManager::Query(const FunctionCallbackInfo& args) { held_lock->mode(), held_lock->client_id()) .ToLocal(&lock_info)) { - Local error_msg = - String::NewFromUtf8(isolate, - "Failed to create lock info object") - .ToLocalChecked(); - resolver->Reject(context, Exception::Error(error_msg)).Check(); + THROW_ERR_OPERATION_FAILED(env, + "Failed to create lock info object"); return; } if (held_list->Set(context, index++, lock_info).IsNothing()) { - Local error_msg = - String::NewFromUtf8(isolate, "Failed to build held locks array") - .ToLocalChecked(); - resolver->Reject(context, Exception::Error(error_msg)).Check(); + THROW_ERR_OPERATION_FAILED(env, "Failed to build held locks array"); return; } } @@ -670,34 +664,21 @@ void LockManager::Query(const FunctionCallbackInfo& args) { pending_request->mode(), pending_request->client_id()) .ToLocal(&lock_info)) { - Local error_msg = - String::NewFromUtf8(isolate, "Failed to create lock info object") - .ToLocalChecked(); - resolver->Reject(context, Exception::Error(error_msg)).Check(); + THROW_ERR_OPERATION_FAILED(env, "Failed to create lock info object"); return; } if (pending_list->Set(context, index++, lock_info).IsNothing()) { - Local error_msg = - String::NewFromUtf8(isolate, - "Failed to build pending locks array") - .ToLocalChecked(); - resolver->Reject(context, Exception::Error(error_msg)).Check(); + THROW_ERR_OPERATION_FAILED(env, + "Failed to build pending locks array"); return; } } } } - if (result->Set(context, FIXED_ONE_BYTE_STRING(isolate, "held"), held_list) - .IsNothing() || - result - ->Set( - context, FIXED_ONE_BYTE_STRING(isolate, "pending"), pending_list) - .IsNothing()) { - Local error_msg = - String::NewFromUtf8(isolate, "Failed to build query result object") - .ToLocalChecked(); - resolver->Reject(context, Exception::Error(error_msg)).Check(); + 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; } @@ -811,7 +792,10 @@ static MaybeLocal CreateLockInfoObject(Isolate* isolate, 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()), @@ -820,8 +804,7 @@ static MaybeLocal CreateLockInfoObject(Isolate* isolate, .ToLocal(&name_string)) { return MaybeLocal(); } - if (obj->Set(context, FIXED_ONE_BYTE_STRING(isolate, "name"), name_string) - .IsNothing()) { + if (obj->Set(context, env->name_string(), name_string).IsNothing()) { return MaybeLocal(); } @@ -832,8 +815,7 @@ static MaybeLocal CreateLockInfoObject(Isolate* isolate, .ToLocal(&mode_string)) { return MaybeLocal(); } - if (obj->Set(context, FIXED_ONE_BYTE_STRING(isolate, "mode"), mode_string) - .IsNothing()) { + if (obj->Set(context, env->mode_string(), mode_string).IsNothing()) { return MaybeLocal(); } @@ -842,10 +824,7 @@ static MaybeLocal CreateLockInfoObject(Isolate* isolate, .ToLocal(&client_id_string)) { return MaybeLocal(); } - if (obj->Set(context, - FIXED_ONE_BYTE_STRING(isolate, "clientId"), - client_id_string) - .IsNothing()) { + if (obj->Set(context, env->clientId_string(), client_id_string).IsNothing()) { return MaybeLocal(); } diff --git a/test/wpt/status/web-locks.json b/test/wpt/status/web-locks.json index 5e5bf3ba197237..8ce8682456d7b4 100644 --- a/test/wpt/status/web-locks.json +++ b/test/wpt/status/web-locks.json @@ -1,4 +1,12 @@ { + "held.https.any.js": { + "fail": { + "flaky": [ + "evaluation in WPTRunner.runJsTests()" + ], + "note": "We may have unhandledRejection during the intentional promise rejection" + } + }, "idlharness.https.any.js": { "fail": { "expected": [ From fd1ac6e5a07ace4719424bf00a2952458e2070b2 Mon Sep 17 00:00:00 2001 From: ishabi Date: Sun, 29 Jun 2025 21:38:07 +0200 Subject: [PATCH 12/16] better memory mngmt and error handling --- src/env_properties.h | 2 +- src/node_locks.cc | 182 ++++++++++++++++++++------------- test/wpt/status/web-locks.json | 8 -- 3 files changed, 112 insertions(+), 80 deletions(-) diff --git a/src/env_properties.h b/src/env_properties.h index 86a3e7b8420070..6e14b5f620c561 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -98,7 +98,7 @@ V(changes_string, "changes") \ V(channel_string, "channel") \ V(chunks_sent_since_last_write_string, "chunksSentSinceLastWrite") \ - V(clientId_string, "clientId") \ + 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 " \ diff --git a/src/node_locks.cc b/src/node_locks.cc index e956b7b7409faa..41c8623089fa5a 100644 --- a/src/node_locks.cc +++ b/src/node_locks.cc @@ -16,6 +16,7 @@ using v8::Exception; using v8::External; using v8::Function; using v8::FunctionCallbackInfo; +using v8::Global; using v8::HandleScope; using v8::Isolate; using v8::Local; @@ -25,11 +26,25 @@ using v8::ObjectTemplate; using v8::Promise; using v8::String; using v8::Value; +using v8::NewStringType; 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(v8::Local ctx, + v8::Local first, + v8::Local second, + v8::Local reason) { + if (first->Reject(ctx, reason).IsNothing()) + return false; + if (second->Reject(ctx, reason).IsNothing()) + return false; + + return true; +} + static MaybeLocal CreateLockInfoObject(Isolate* isolate, Local context, const std::u16string& name, @@ -88,7 +103,7 @@ bool LockManager::IsGrantable(const LockRequest* request) const { // Called when the user callback promise fulfills static void OnLockCallbackFulfilled( - const v8::FunctionCallbackInfo& info) { + const FunctionCallbackInfo& info) { HandleScope handle_scope(info.GetIsolate()); Environment* env = Environment::GetCurrent(info); @@ -105,7 +120,7 @@ static void OnLockCallbackFulfilled( // Called when the user callback promise rejects static void OnLockCallbackRejected( - const v8::FunctionCallbackInfo& info) { + const FunctionCallbackInfo& info) { HandleScope handle_scope(info.GetIsolate()); Environment* env = Environment::GetCurrent(info); @@ -120,25 +135,26 @@ static void OnLockCallbackRejected( // Called when the promise returned from the user's callback resolves static void OnIfAvailableFulfill( - const v8::FunctionCallbackInfo& info) { + const FunctionCallbackInfo& info) { HandleScope handle_scope(info.GetIsolate()); - auto* holder = static_cast*>( + auto* holder = static_cast*>( info.Data().As()->Value()); USE(holder->Get(info.GetIsolate()) ->Resolve(info.GetIsolate()->GetCurrentContext(), info[0])); - + holder->Reset(); delete holder; } // Called when the promise returned from the user's callback rejects static void OnIfAvailableReject( - const v8::FunctionCallbackInfo& info) { + const FunctionCallbackInfo& info) { HandleScope handle_scope(info.GetIsolate()); - auto* holder = static_cast*>( + auto* holder = static_cast*>( info.Data().As()->Value()); USE(holder->Get(info.GetIsolate()) ->Reject(info.GetIsolate()->GetCurrentContext(), info[0])); - + holder->Reset(); + delete holder; } @@ -305,61 +321,72 @@ void LockManager::ProcessQueue(Environment* env) { if (!if_available_request->callback() ->Call(context, Undefined(isolate), 1, &null_arg) .ToLocal(&callback_result)) { - USE(if_available_request->waiting_promise()->Reject( - context, try_catch_scope.Exception())); - USE(if_available_request->released_promise()->Reject( - context, try_catch_scope.Exception())); + if (!RejectBoth(context, + if_available_request->waiting_promise(), + if_available_request->released_promise(), + try_catch_scope.Exception())) + return; return; } } if (callback_result->IsPromise()) { Local p = callback_result.As(); - // Use a Global holder so the resolver survives until the promise - // settles. - auto* fulf_holder = new v8::Global( + // The resolver survives until the promise settles and is freed automatically. + auto resolve_holder = std::make_unique>( isolate, if_available_request->released_promise()); - auto* rej_holder = new v8::Global( + auto reject_holder = std::make_unique>( isolate, if_available_request->released_promise()); Local on_fulfilled; Local on_rejected; CHECK(Function::New(context, OnIfAvailableFulfill, - External::New(isolate, fulf_holder)) + External::New(isolate, resolve_holder.get())) .ToLocal(&on_fulfilled)); CHECK(Function::New(context, OnIfAvailableReject, - External::New(isolate, rej_holder)) + External::New(isolate, reject_holder.get())) .ToLocal(&on_rejected)); - if (p->Then(context, on_fulfilled, on_rejected).IsEmpty()) { - // Attaching handlers failed; reject promises and return. - Local err_str_local = - String::NewFromUtf8(isolate, "Failed to attach promise handlers") - .ToLocalChecked(); - Local err_val = Exception::Error(err_str_local); + // Transfer ownership to the callbacks. + resolve_holder.release(); + reject_holder.release(); - USE(if_available_request->waiting_promise()->Reject(context, - err_val)); - USE(if_available_request->released_promise()->Reject(context, - err_val)); + { + TryCatchScope try_catch_scope(env); + if (p->Then(context, on_fulfilled, on_rejected).IsEmpty()) { + if (!try_catch_scope.CanContinue()) + return; - return; + Local err_val; + if (try_catch_scope.HasCaught() && !try_catch_scope.Exception().IsEmpty()) { + err_val = try_catch_scope.Exception(); + } else { + err_val = Exception::Error(FIXED_ONE_BYTE_STRING(isolate, "Failed to attach promise handlers")); + } + + 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. - USE(if_available_request->waiting_promise()->Resolve(context, p)); + if (if_available_request->waiting_promise()->Resolve(context, p).IsNothing()) + return; return; } // Non-promise callback result: settle both promises right away. - USE(if_available_request->waiting_promise()->Resolve(context, - callback_result)); - USE(if_available_request->released_promise()->Resolve(context, - callback_result)); + if (if_available_request->waiting_promise()->Resolve(context, callback_result).IsNothing()) + return; + if (if_available_request->released_promise()->Resolve(context, callback_result).IsNothing()) + return; return; } @@ -394,7 +421,8 @@ void LockManager::ProcessQueue(Environment* env) { } Local error = Exception::Error(error_string); - USE(existing_lock->released_promise()->Reject(context, error)); + if (existing_lock->released_promise()->Reject(context, error).IsNothing()) + return; } // Remove stolen locks from current environment immediately @@ -435,85 +463,95 @@ void LockManager::ProcessQueue(Environment* env) { } // Create and store the new granted lock - Local lock_info_obj; + Local lock_info; if (!CreateLockInfoObject(isolate, context, grantable_request->name(), grantable_request->mode(), grantable_request->client_id()) - .ToLocal(&lock_info_obj)) { + .ToLocal(&lock_info)) { return; } // Call user callback - Local callback_arg = lock_info_obj; + Local callback_arg = lock_info; Local callback_result; { TryCatchScope try_catch_scope(env); if (!grantable_request->callback() ->Call(context, Undefined(isolate), 1, &callback_arg) .ToLocal(&callback_result)) { - USE(grantable_request->waiting_promise()->Reject( - context, try_catch_scope.Exception())); - USE(grantable_request->released_promise()->Reject( - context, try_catch_scope.Exception())); - + if (!RejectBoth(context, + grantable_request->waiting_promise(), + grantable_request->released_promise(), + try_catch_scope.Exception())) + return; return; } } // Allocate a LockHolder on the heap to safely manage the lock's lifetime // until the user's callback promise settles. - auto* fulfill_holder = new LockHolder(granted_lock); - auto* reject_holder = new LockHolder(granted_lock); + auto lock_resolve_holder = std::make_unique(granted_lock); + auto lock_reject_holder = std::make_unique(granted_lock); Local on_fulfilled_callback; Local on_rejected_callback; // Create fulfilled callback first if (!Function::New(context, OnLockCallbackFulfilled, - External::New(isolate, fulfill_holder)) + External::New(isolate, lock_resolve_holder.get())) .ToLocal(&on_fulfilled_callback)) { - delete fulfill_holder; - delete reject_holder; return; } // Create rejected callback second if (!Function::New(context, OnLockCallbackRejected, - External::New(isolate, reject_holder)) + External::New(isolate, lock_reject_holder.get())) .ToLocal(&on_rejected_callback)) { - // fulfill_holder is now owned by the External, don't delete it - delete reject_holder; + lock_resolve_holder.release(); + return; } + lock_resolve_holder.release(); + lock_reject_holder.release(); + // Handle promise chain if (callback_result->IsPromise()) { Local promise = callback_result.As(); - if (promise->Then(context, on_fulfilled_callback, on_rejected_callback) - .IsEmpty()) { - // Then() failed, reject both promises and return - Local err_str_local = - String::NewFromUtf8(isolate, "Failed to attach promise handlers") - .ToLocalChecked(); - Local err_val = Exception::Error(err_str_local); + { + TryCatchScope try_catch_scope(env); + if (promise->Then(context, on_fulfilled_callback, on_rejected_callback).IsEmpty()) { + if (!try_catch_scope.CanContinue()) + return; - USE(grantable_request->waiting_promise()->Reject(context, err_val)); - USE(grantable_request->released_promise()->Reject(context, err_val)); + Local err_val; + if (try_catch_scope.HasCaught() && !try_catch_scope.Exception().IsEmpty()) { + err_val = try_catch_scope.Exception(); + } else { + err_val = Exception::Error(FIXED_ONE_BYTE_STRING(isolate, "Failed to attach promise handlers")); + } - return; + RejectBoth(context, + grantable_request->waiting_promise(), + grantable_request->released_promise(), + err_val); + return; + } } // Lock granted: waiting_promise resolves now with the promise returned // by the callback; on_fulfilled/on_rejected will release the lock when // that promise settles. - USE(grantable_request->waiting_promise()->Resolve(context, - callback_result)); + if (grantable_request->waiting_promise()->Resolve(context, callback_result).IsNothing()) { + return; + } } else { - USE(grantable_request->waiting_promise()->Resolve(context, - callback_result)); + if (grantable_request->waiting_promise()->Resolve(context, callback_result).IsNothing()) { + return; + } Local promise_args[] = {callback_result}; if (on_fulfilled_callback ->Call(context, Undefined(isolate), 1, promise_args) @@ -523,7 +561,6 @@ void LockManager::ProcessQueue(Environment* env) { // callback return; } - delete reject_holder; } } } @@ -682,7 +719,8 @@ void LockManager::Query(const FunctionCallbackInfo& args) { return; } - USE(resolver->Resolve(context, result)); + if (resolver->Resolve(context, result).IsNothing()) + return; } // Runs after the user callback (or its returned promise) settles. @@ -702,10 +740,12 @@ void LockManager::ReleaseLockAndProcessQueue(Environment* env, if (!lock->is_stolen()) { if (was_rejected) { // Propagate rejection from the user callback - USE(lock->released_promise()->Reject(context, callback_result)); + if (lock->released_promise()->Reject(context, callback_result).IsNothing()) + return; } else { // Propagate fulfilment - USE(lock->released_promise()->Resolve(context, callback_result)); + if (lock->released_promise()->Resolve(context, callback_result).IsNothing()) + return; } } @@ -799,7 +839,7 @@ static MaybeLocal CreateLockInfoObject(Isolate* isolate, Local name_string; if (!String::NewFromTwoByte(isolate, reinterpret_cast(name.data()), - v8::NewStringType::kNormal, + NewStringType::kNormal, static_cast(name.length())) .ToLocal(&name_string)) { return MaybeLocal(); @@ -824,7 +864,7 @@ static MaybeLocal CreateLockInfoObject(Isolate* isolate, .ToLocal(&client_id_string)) { return MaybeLocal(); } - if (obj->Set(context, env->clientId_string(), client_id_string).IsNothing()) { + if (obj->Set(context, env->client_id_string(), client_id_string).IsNothing()) { return MaybeLocal(); } diff --git a/test/wpt/status/web-locks.json b/test/wpt/status/web-locks.json index 8ce8682456d7b4..5e5bf3ba197237 100644 --- a/test/wpt/status/web-locks.json +++ b/test/wpt/status/web-locks.json @@ -1,12 +1,4 @@ { - "held.https.any.js": { - "fail": { - "flaky": [ - "evaluation in WPTRunner.runJsTests()" - ], - "note": "We may have unhandledRejection during the intentional promise rejection" - } - }, "idlharness.https.any.js": { "fail": { "expected": [ From 4d9169f34a47cdaa996cf90afd64c2c6d006cdd2 Mon Sep 17 00:00:00 2001 From: ishabi Date: Sun, 29 Jun 2025 21:44:56 +0200 Subject: [PATCH 13/16] fix linter --- src/node_locks.cc | 86 +++++++++++++++++++++++++++-------------------- 1 file changed, 50 insertions(+), 36 deletions(-) diff --git a/src/node_locks.cc b/src/node_locks.cc index 41c8623089fa5a..c7f50d6f81b46b 100644 --- a/src/node_locks.cc +++ b/src/node_locks.cc @@ -21,12 +21,12 @@ using v8::HandleScope; using v8::Isolate; using v8::Local; using v8::MaybeLocal; +using v8::NewStringType; using v8::Object; using v8::ObjectTemplate; using v8::Promise; using v8::String; using v8::Value; -using v8::NewStringType; static constexpr const char* kSharedMode = "shared"; static constexpr const char* kExclusiveMode = "exclusive"; @@ -37,10 +37,8 @@ static bool RejectBoth(v8::Local ctx, v8::Local first, v8::Local second, v8::Local reason) { - if (first->Reject(ctx, reason).IsNothing()) - return false; - if (second->Reject(ctx, reason).IsNothing()) - return false; + if (first->Reject(ctx, reason).IsNothing()) return false; + if (second->Reject(ctx, reason).IsNothing()) return false; return true; } @@ -102,8 +100,7 @@ bool LockManager::IsGrantable(const LockRequest* request) const { } // Called when the user callback promise fulfills -static void OnLockCallbackFulfilled( - const FunctionCallbackInfo& info) { +static void OnLockCallbackFulfilled(const FunctionCallbackInfo& info) { HandleScope handle_scope(info.GetIsolate()); Environment* env = Environment::GetCurrent(info); @@ -119,8 +116,7 @@ static void OnLockCallbackFulfilled( } // Called when the user callback promise rejects -static void OnLockCallbackRejected( - const FunctionCallbackInfo& info) { +static void OnLockCallbackRejected(const FunctionCallbackInfo& info) { HandleScope handle_scope(info.GetIsolate()); Environment* env = Environment::GetCurrent(info); @@ -134,8 +130,7 @@ static void OnLockCallbackRejected( } // Called when the promise returned from the user's callback resolves -static void OnIfAvailableFulfill( - const FunctionCallbackInfo& info) { +static void OnIfAvailableFulfill(const FunctionCallbackInfo& info) { HandleScope handle_scope(info.GetIsolate()); auto* holder = static_cast*>( info.Data().As()->Value()); @@ -146,15 +141,14 @@ static void OnIfAvailableFulfill( } // Called when the promise returned from the user's callback rejects -static void OnIfAvailableReject( - const FunctionCallbackInfo& info) { +static void OnIfAvailableReject(const FunctionCallbackInfo& info) { HandleScope handle_scope(info.GetIsolate()); auto* holder = static_cast*>( info.Data().As()->Value()); USE(holder->Get(info.GetIsolate()) ->Reject(info.GetIsolate()->GetCurrentContext(), info[0])); holder->Reset(); - + delete holder; } @@ -332,7 +326,8 @@ void LockManager::ProcessQueue(Environment* env) { if (callback_result->IsPromise()) { Local p = callback_result.As(); - // The resolver survives until the promise settles and is freed automatically. + // The resolver survives until the promise settles and is freed + // automatically. auto resolve_holder = std::make_unique>( isolate, if_available_request->released_promise()); auto reject_holder = std::make_unique>( @@ -356,14 +351,15 @@ void LockManager::ProcessQueue(Environment* env) { { TryCatchScope try_catch_scope(env); if (p->Then(context, on_fulfilled, on_rejected).IsEmpty()) { - if (!try_catch_scope.CanContinue()) - return; + if (!try_catch_scope.CanContinue()) return; Local err_val; - if (try_catch_scope.HasCaught() && !try_catch_scope.Exception().IsEmpty()) { + if (try_catch_scope.HasCaught() && + !try_catch_scope.Exception().IsEmpty()) { err_val = try_catch_scope.Exception(); } else { - err_val = Exception::Error(FIXED_ONE_BYTE_STRING(isolate, "Failed to attach promise handlers")); + err_val = Exception::Error(FIXED_ONE_BYTE_STRING( + isolate, "Failed to attach promise handlers")); } RejectBoth(context, @@ -376,16 +372,22 @@ void LockManager::ProcessQueue(Environment* env) { // After handlers are attached, resolve waiting_promise with the // promise. - if (if_available_request->waiting_promise()->Resolve(context, p).IsNothing()) + if (if_available_request->waiting_promise() + ->Resolve(context, p) + .IsNothing()) return; return; } // Non-promise callback result: settle both promises right away. - if (if_available_request->waiting_promise()->Resolve(context, callback_result).IsNothing()) + if (if_available_request->waiting_promise() + ->Resolve(context, callback_result) + .IsNothing()) return; - if (if_available_request->released_promise()->Resolve(context, callback_result).IsNothing()) + if (if_available_request->released_promise() + ->Resolve(context, callback_result) + .IsNothing()) return; return; @@ -421,7 +423,9 @@ void LockManager::ProcessQueue(Environment* env) { } Local error = Exception::Error(error_string); - if (existing_lock->released_promise()->Reject(context, error).IsNothing()) + if (existing_lock->released_promise() + ->Reject(context, error) + .IsNothing()) return; } @@ -511,7 +515,7 @@ void LockManager::ProcessQueue(Environment* env) { External::New(isolate, lock_reject_holder.get())) .ToLocal(&on_rejected_callback)) { lock_resolve_holder.release(); - + return; } @@ -523,15 +527,17 @@ void LockManager::ProcessQueue(Environment* env) { Local promise = callback_result.As(); { TryCatchScope try_catch_scope(env); - if (promise->Then(context, on_fulfilled_callback, on_rejected_callback).IsEmpty()) { - if (!try_catch_scope.CanContinue()) - return; + if (promise->Then(context, on_fulfilled_callback, on_rejected_callback) + .IsEmpty()) { + if (!try_catch_scope.CanContinue()) return; Local err_val; - if (try_catch_scope.HasCaught() && !try_catch_scope.Exception().IsEmpty()) { + if (try_catch_scope.HasCaught() && + !try_catch_scope.Exception().IsEmpty()) { err_val = try_catch_scope.Exception(); } else { - err_val = Exception::Error(FIXED_ONE_BYTE_STRING(isolate, "Failed to attach promise handlers")); + err_val = Exception::Error(FIXED_ONE_BYTE_STRING( + isolate, "Failed to attach promise handlers")); } RejectBoth(context, @@ -545,11 +551,15 @@ void LockManager::ProcessQueue(Environment* env) { // Lock granted: waiting_promise resolves now with the promise returned // by the callback; on_fulfilled/on_rejected will release the lock when // that promise settles. - if (grantable_request->waiting_promise()->Resolve(context, callback_result).IsNothing()) { + if (grantable_request->waiting_promise() + ->Resolve(context, callback_result) + .IsNothing()) { return; } } else { - if (grantable_request->waiting_promise()->Resolve(context, callback_result).IsNothing()) { + if (grantable_request->waiting_promise() + ->Resolve(context, callback_result) + .IsNothing()) { return; } Local promise_args[] = {callback_result}; @@ -719,8 +729,7 @@ void LockManager::Query(const FunctionCallbackInfo& args) { return; } - if (resolver->Resolve(context, result).IsNothing()) - return; + if (resolver->Resolve(context, result).IsNothing()) return; } // Runs after the user callback (or its returned promise) settles. @@ -740,11 +749,15 @@ void LockManager::ReleaseLockAndProcessQueue(Environment* env, if (!lock->is_stolen()) { if (was_rejected) { // Propagate rejection from the user callback - if (lock->released_promise()->Reject(context, callback_result).IsNothing()) + if (lock->released_promise() + ->Reject(context, callback_result) + .IsNothing()) return; } else { // Propagate fulfilment - if (lock->released_promise()->Resolve(context, callback_result).IsNothing()) + if (lock->released_promise() + ->Resolve(context, callback_result) + .IsNothing()) return; } } @@ -864,7 +877,8 @@ static MaybeLocal CreateLockInfoObject(Isolate* isolate, .ToLocal(&client_id_string)) { return MaybeLocal(); } - if (obj->Set(context, env->client_id_string(), client_id_string).IsNothing()) { + if (obj->Set(context, env->client_id_string(), client_id_string) + .IsNothing()) { return MaybeLocal(); } From 6ee78acedd068df7b7a497a964885dbf062bf223 Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 16 Jul 2025 16:38:58 +0200 Subject: [PATCH 14/16] remove unnecessary external and global --- src/node_locks.cc | 45 ++++++++++----------------------------------- 1 file changed, 10 insertions(+), 35 deletions(-) diff --git a/src/node_locks.cc b/src/node_locks.cc index c7f50d6f81b46b..91b4c7c0239aca 100644 --- a/src/node_locks.cc +++ b/src/node_locks.cc @@ -16,7 +16,6 @@ using v8::Exception; using v8::External; using v8::Function; using v8::FunctionCallbackInfo; -using v8::Global; using v8::HandleScope; using v8::Isolate; using v8::Local; @@ -104,11 +103,9 @@ static void OnLockCallbackFulfilled(const FunctionCallbackInfo& info) { HandleScope handle_scope(info.GetIsolate()); Environment* env = Environment::GetCurrent(info); - // Extract the LockHolder from V8 External data - auto* lock_holder = - static_cast(info.Data().As()->Value()); + std::unique_ptr lock_holder{ + static_cast(info.Data().As()->Value())}; std::shared_ptr lock = lock_holder->lock(); - delete lock_holder; // Release the lock and continue processing the queue. LockManager::GetCurrent()->ReleaseLockAndProcessQueue( @@ -120,10 +117,9 @@ static void OnLockCallbackRejected(const FunctionCallbackInfo& info) { HandleScope handle_scope(info.GetIsolate()); Environment* env = Environment::GetCurrent(info); - auto* lock_holder = - static_cast(info.Data().As()->Value()); + std::unique_ptr lock_holder{ + static_cast(info.Data().As()->Value())}; std::shared_ptr lock = lock_holder->lock(); - delete lock_holder; LockManager::GetCurrent()->ReleaseLockAndProcessQueue( env, lock, info[0], true); @@ -132,24 +128,14 @@ static void OnLockCallbackRejected(const FunctionCallbackInfo& info) { // Called when the promise returned from the user's callback resolves static void OnIfAvailableFulfill(const FunctionCallbackInfo& info) { HandleScope handle_scope(info.GetIsolate()); - auto* holder = static_cast*>( - info.Data().As()->Value()); - USE(holder->Get(info.GetIsolate()) - ->Resolve(info.GetIsolate()->GetCurrentContext(), info[0])); - holder->Reset(); - delete holder; + USE(info.Data().As()->Resolve( + info.GetIsolate()->GetCurrentContext(), info[0])); } // Called when the promise returned from the user's callback rejects static void OnIfAvailableReject(const FunctionCallbackInfo& info) { - HandleScope handle_scope(info.GetIsolate()); - auto* holder = static_cast*>( - info.Data().As()->Value()); - USE(holder->Get(info.GetIsolate()) - ->Reject(info.GetIsolate()->GetCurrentContext(), info[0])); - holder->Reset(); - - delete holder; + USE(info.Data().As()->Reject( + info.GetIsolate()->GetCurrentContext(), info[0])); } void LockManager::CleanupStolenLocks(Environment* env) { @@ -326,28 +312,17 @@ void LockManager::ProcessQueue(Environment* env) { if (callback_result->IsPromise()) { Local p = callback_result.As(); - // The resolver survives until the promise settles and is freed - // automatically. - auto resolve_holder = std::make_unique>( - isolate, if_available_request->released_promise()); - auto reject_holder = std::make_unique>( - isolate, if_available_request->released_promise()); - Local on_fulfilled; Local on_rejected; CHECK(Function::New(context, OnIfAvailableFulfill, - External::New(isolate, resolve_holder.get())) + if_available_request->released_promise()) .ToLocal(&on_fulfilled)); CHECK(Function::New(context, OnIfAvailableReject, - External::New(isolate, reject_holder.get())) + if_available_request->released_promise()) .ToLocal(&on_rejected)); - // Transfer ownership to the callbacks. - resolve_holder.release(); - reject_holder.release(); - { TryCatchScope try_catch_scope(env); if (p->Then(context, on_fulfilled, on_rejected).IsEmpty()) { From 491b394178a86a404bcca5f71ed28c0ca63585ed Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 16 Jul 2025 16:51:28 +0200 Subject: [PATCH 15/16] fix js linter --- test/parallel/test-web-locks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/parallel/test-web-locks.js b/test/parallel/test-web-locks.js index 4fe16392b8fe74..e938bf89f3a52b 100644 --- a/test/parallel/test-web-locks.js +++ b/test/parallel/test-web-locks.js @@ -184,7 +184,7 @@ describe('Web Locks with worker threads', () => { .request('als-context-test', async () => { assert.strictEqual(als.getStore(), store); }) - .catch(common.mustNotCall()); + .then(common.mustCall()); }); }); From 4dcb0272bc4bcd918f00f7482f26fd6809530ff7 Mon Sep 17 00:00:00 2001 From: ishabi Date: Wed, 16 Jul 2025 23:21:40 +0200 Subject: [PATCH 16/16] use baseObject --- src/env_properties.h | 1 + src/node_locks.cc | 68 +++++++++++++++++++++++++++++--------------- src/node_locks.h | 21 ++++++++++++-- 3 files changed, 65 insertions(+), 25 deletions(-) diff --git a/src/env_properties.h b/src/env_properties.h index 6e14b5f620c561..230eb9986198ef 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -453,6 +453,7 @@ V(intervalhistogram_constructor_template, v8::FunctionTemplate) \ V(js_transferable_constructor_template, v8::FunctionTemplate) \ V(libuv_stream_wrap_ctor_template, v8::FunctionTemplate) \ + V(lock_holder_constructor_template, v8::FunctionTemplate) \ V(message_port_constructor_template, v8::FunctionTemplate) \ V(module_wrap_constructor_template, v8::FunctionTemplate) \ V(microtask_queue_ctor_template, v8::FunctionTemplate) \ diff --git a/src/node_locks.cc b/src/node_locks.cc index 91b4c7c0239aca..0c2dc7ff0bb2ff 100644 --- a/src/node_locks.cc +++ b/src/node_locks.cc @@ -1,5 +1,6 @@ #include "node_locks.h" +#include "base_object-inl.h" #include "env-inl.h" #include "node_errors.h" #include "node_external_reference.h" @@ -13,9 +14,9 @@ using node::errors::TryCatchScope; using v8::Array; using v8::Context; using v8::Exception; -using v8::External; using v8::Function; using v8::FunctionCallbackInfo; +using v8::FunctionTemplate; using v8::HandleScope; using v8::Isolate; using v8::Local; @@ -32,10 +33,10 @@ static constexpr const char* kExclusiveMode = "exclusive"; static constexpr const char* kLockStolenError = "LOCK_STOLEN"; // Reject two promises and return `false` on failure. -static bool RejectBoth(v8::Local ctx, - v8::Local first, - v8::Local second, - v8::Local reason) { +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; @@ -103,8 +104,8 @@ static void OnLockCallbackFulfilled(const FunctionCallbackInfo& info) { HandleScope handle_scope(info.GetIsolate()); Environment* env = Environment::GetCurrent(info); - std::unique_ptr lock_holder{ - static_cast(info.Data().As()->Value())}; + BaseObjectPtr lock_holder{ + BaseObject::FromJSObject(info.Data())}; std::shared_ptr lock = lock_holder->lock(); // Release the lock and continue processing the queue. @@ -117,8 +118,8 @@ static void OnLockCallbackRejected(const FunctionCallbackInfo& info) { HandleScope handle_scope(info.GetIsolate()); Environment* env = Environment::GetCurrent(info); - std::unique_ptr lock_holder{ - static_cast(info.Data().As()->Value())}; + BaseObjectPtr lock_holder{ + BaseObject::FromJSObject(info.Data())}; std::shared_ptr lock = lock_holder->lock(); LockManager::GetCurrent()->ReleaseLockAndProcessQueue( @@ -469,34 +470,27 @@ void LockManager::ProcessQueue(Environment* env) { } } - // Allocate a LockHolder on the heap to safely manage the lock's lifetime + // Create LockHolder BaseObjects to safely manage the lock's lifetime // until the user's callback promise settles. - auto lock_resolve_holder = std::make_unique(granted_lock); - auto lock_reject_holder = std::make_unique(granted_lock); + auto lock_resolve_holder = LockHolder::Create(env, granted_lock); + auto lock_reject_holder = LockHolder::Create(env, granted_lock); Local on_fulfilled_callback; Local on_rejected_callback; // Create fulfilled callback first - if (!Function::New(context, - OnLockCallbackFulfilled, - External::New(isolate, lock_resolve_holder.get())) + if (!Function::New( + context, OnLockCallbackFulfilled, lock_resolve_holder->object()) .ToLocal(&on_fulfilled_callback)) { return; } // Create rejected callback second - if (!Function::New(context, - OnLockCallbackRejected, - External::New(isolate, lock_reject_holder.get())) + if (!Function::New( + context, OnLockCallbackRejected, lock_reject_holder->object()) .ToLocal(&on_rejected_callback)) { - lock_resolve_holder.release(); - return; } - lock_resolve_holder.release(); - lock_reject_holder.release(); - // Handle promise chain if (callback_result->IsPromise()) { Local promise = callback_result.As(); @@ -899,6 +893,34 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(OnIfAvailableReject); } +BaseObjectPtr LockHolder::Create(Environment* env, + std::shared_ptr lock) { + Local obj; + if (!GetConstructorTemplate(env) + ->InstanceTemplate() + ->NewInstance(env->context()) + .ToLocal(&obj)) { + return nullptr; + } + + return MakeBaseObject(env, obj, std::move(lock)); +} + +Local LockHolder::GetConstructorTemplate(Environment* env) { + IsolateData* isolate_data = env->isolate_data(); + Local tmpl = + isolate_data->lock_holder_constructor_template(); + if (tmpl.IsEmpty()) { + Isolate* isolate = isolate_data->isolate(); + tmpl = NewFunctionTemplate(isolate, nullptr); + tmpl->SetClassName(FIXED_ONE_BYTE_STRING(isolate, "LockHolder")); + tmpl->InstanceTemplate()->SetInternalFieldCount( + LockHolder::kInternalFieldCount); + isolate_data->set_lock_holder_constructor_template(tmpl); + } + return tmpl; +} + } // namespace node::worker::locks NODE_BINDING_CONTEXT_AWARE_INTERNAL( diff --git a/src/node_locks.h b/src/node_locks.h index 5291baad8f15b5..68a2d84ff38419 100644 --- a/src/node_locks.h +++ b/src/node_locks.h @@ -8,6 +8,7 @@ #include #include +#include "base_object.h" #include "env.h" #include "node_mutex.h" #include "v8.h" @@ -62,9 +63,15 @@ class Lock final { v8::Global released_promise_; }; -class LockHolder final { +class LockHolder final : public BaseObject { public: - explicit LockHolder(std::shared_ptr lock) : lock_(std::move(lock)) {} + LockHolder(Environment* env, + v8::Local obj, + std::shared_ptr lock) + : BaseObject(env, obj), lock_(std::move(lock)) { + MakeWeak(); + } + ~LockHolder() = default; LockHolder(const LockHolder&) = delete; @@ -72,7 +79,17 @@ class LockHolder final { std::shared_ptr lock() const { return lock_; } + SET_NO_MEMORY_INFO() + SET_MEMORY_INFO_NAME(LockHolder) + SET_SELF_SIZE(LockHolder) + + static BaseObjectPtr Create(Environment* env, + std::shared_ptr lock); + private: + static v8::Local GetConstructorTemplate( + Environment* env); + std::shared_ptr lock_; };