-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
worker: implement Web Locks API #36502
base: main
Are you sure you want to change the base?
Conversation
Nice. Did you happen to benchmark this relative to the native addon alternative, in particular for the cross-worker cases? I'd be interested in seeing how the lock acquisition time compares and see if the use of Left a few comments on the test but otherwise it's looking good! |
would it be possible to pull in the wpt tests? |
I'm trying it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few nits, ran a few simulations in my head to see that it works and ran code a bunch.
I am sure there are edge cases I haven't considered and there are probably edge cases and bugs in the impl - but I think we should land this (as experimental) and iterate on it since this sort of concurrency code is hard to review but good to iterate on.
locksInitialized = true; | ||
// Make sure that locks are initialized before active | ||
// multithreading starts. | ||
require('worker_threads').locks.query(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny nit: This is kind of implicit - I would prefer it if this was more explicit (like .locks.initialize
that calls query internally).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well … I can add another method, but I don’t think .locks.initialize()
as a name would work since that’s public, and we could use a symbol, but at that point we’re probably introducing more complexity than the comment here provides?
// on the main thread makes sure that locks will be initialized. | ||
assert(isMainThread); | ||
const sab = new SharedArrayBuffer(16); | ||
this.state = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is kind of confusing but I can't think of a nicer way to maintain this state.
lib/internal/worker/locks.js
Outdated
// internal state. | ||
const mutex = this.state.mutexOwner; | ||
let owner; | ||
while ((owner = Atomics.compareExchange(mutex, 0, -1, threadId)) !== -1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally there would be less "magic numbers" and more constants here (they're codified in comments atm) but I'm not sure that'd actually make things clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve replaced the -1
s with MUTEX_UNOWNED
, that should be a bit more expressive. Are there other constants that you’d like to see gone? :)
There are many failures. One that I see a lot is related to thrown errors:
|
One of the failing tests has something like this, which results in an internal error: const { locks } = require('worker_threads');
const controller = new AbortController();
const p = locks.request('name', {signal: controller.signal}, lock => { console.log('got lock') })
controller.abort(); |
This is based upon nodejs#22719 and exposes the same API. Instead of a C++-backed approach, this variant implements the API mostly in JS. Refs: nodejs#22719 Co-authored-by: Gus Caplan <me@gus.host>
I added a commit with WPT (force-pushed to include #36501) Current results:
|
@addaleax... just running a quick benchmark run here... first scenario... time acuiring the lock over and over again 'use strict';
const common = require('../common');
const piscina_locks = require('../../../../nearform/piscina-locks');
const { Worker, isMainThread, parentPort, locks } = require('worker_threads');
const bench = common.createBenchmark(main, {
n: [1e4],
type: ['js', 'native']
});
async function testWithPiscina(n) {
bench.start();
for (let i = 0; i < n; i++) {
await piscina_locks.request('foo', () => {});
}
bench.end(n);
}
async function testWithJS(n) {
bench.start();
for (let i = 0; i < n; i++) {
await locks.request('foo', () => {});
}
bench.end(n);
}
async function main({ n, type }) {
switch (type) {
case 'js': await testWithJS(n); break;
case 'native': await testWithPiscina(n); break;
}
} Benchmark results are:
second scenario... launch a worker, post a message, when the worker receives the message it acquires a lock a posts the message back to the main thread. 'use strict';
const common = require('../common');
const piscina_locks = require('../../../../nearform/piscina-locks');
const { Worker, isMainThread, parentPort, locks } = require('worker_threads');
if (isMainThread) {
const bench = common.createBenchmark(main, {
n: [1e4],
type: ['js', 'native']
});
async function testWithPiscina(w, n) {
bench.start();
for (let i = 0; i < n; i++) {
let res;
const p = new Promise((resolve) => {
res = resolve;
});
w.once('message', (msg) => {
if (msg === 'ok') res();
});
w.postMessage('native');
await p;
}
bench.end(n);
}
async function testWithJS(w, n) {
bench.start();
for (let i = 0; i < n; i++) {
let res;
const p = new Promise((resolve) => {
res = resolve;
});
w.once('message', (msg) => {
if (msg === 'ok') res();
});
w.postMessage('js');
await p;
}
bench.end(n);
}
async function main({ n, type }) {
const w = new Worker(__filename);
switch (type) {
case 'js': await testWithJS(w, n); break;
case 'native': await testWithPiscina(w, n); break;
}
w.terminate();
}
} else {
parentPort.on('message', (type) => {
switch (type) {
case 'js':
locks.request('foo', () => {
parentPort.postMessage('ok');
});
break;
case 'native':
piscina_locks.request('foo', () => {
parentPort.postMessage('ok');
});
}
});
} Benchmark results are:
The JS version results are good but definitely lag behind the native approach that piscina-locks takes. Not sure that there's room for much more improvement in the JS version. It's also worth noting that the internals of the JS version are quite a bit more complicated than the native approach. None of this is to block, of course, but just to add to the conversation. |
// multithreading starts. | ||
require('worker_threads').locks.query(); | ||
} | ||
|
||
// Set up the C++ handle for the worker, as well as some internal wiring. | ||
this[kHandle] = new WorkerImpl(url, | ||
env === process.env ? null : env, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
env === process.env ? null : env,
Hey @addaleax ! I came across this PR and am willing to give try to this, I managed to resolve all conflicts locally and updated the WPTs should I push the changes to this branch? or open a fresh PR? Also when running the latest WPTs somehow the process completely hangs and tests dont proceed at all does anyone know why? Also to look at what the present state looks like after all conflicts resolved you can checkout https://github.com/nodejs/node/compare/main...debadree25:node:web-locks-v3?expand=1 |
Ok weirdly seems like removing "acquire.tentative.https.any.js": {
"skip": "Unexpected global object"
}, from test/wpt/status/web-locks.json makes the tests run and following is the present status Path: wpt/test-web-locks
---- held ----
[SKIPPED] Error: this uncaught rejection is expected
---- idlharness ----
[SKIPPED] Unexpected global object
---- non-secure-context ----
[SKIPPED] navigator.locks is only present in secure contexts
---- query ----
[SKIPPED] Uses Web Workers
---- resource-names ----
[SKIPPED] Unexpected global object
---- signal ----
[SKIPPED] Various errors. Process hangs.
---- steal ----
[SKIPPED] AbortError: The operation was aborted
---- storage-buckets ----
[SKIPPED] Storage not in wpt
---- Lock request with ifAvailable - lock available ----
[PASS] Lock request with ifAvailable - lock available
---- Lock request with ifAvailable - lock not available ----
[PASS] Lock request with ifAvailable - lock not available
---- navigator.locks.request requires a name and a callback ----
[PASS] navigator.locks.request requires a name and a callback
---- Lock request with ifAvailable - lock not available, callback throws ----
[PASS] Lock request with ifAvailable - lock not available, callback throws
---- mode must be "shared" or "exclusive" ----
[PASS] mode must be "shared" or "exclusive"
---- Lock request with ifAvailable - unrelated lock held ----
[PASS] Lock request with ifAvailable - unrelated lock held
---- Shared lock request with ifAvailable - shared lock held ----
[PASS] Shared lock request with ifAvailable - shared lock held
---- The 'steal' and 'ifAvailable' options are mutually exclusive ----
[PASS] The 'steal' and 'ifAvailable' options are mutually exclusive
---- The 'steal' option must be used with exclusive locks ----
[PASS] The 'steal' option must be used with exclusive locks
---- Exclusive lock request with ifAvailable - shared lock held ----
[PASS] Exclusive lock request with ifAvailable - shared lock held
---- The 'signal' and 'steal' options are mutually exclusive ----
[PASS] The 'signal' and 'steal' options are mutually exclusive
---- Shared lock request with ifAvailable - exclusive lock held ----
[PASS] Shared lock request with ifAvailable - exclusive lock held
---- The 'signal' and 'ifAvailable' options are mutually exclusive ----
[PASS] The 'signal' and 'ifAvailable' options are mutually exclusive
---- Lock attributes reflect requested properties (exclusive) ----
[PASS] Lock attributes reflect requested properties (exclusive)
---- Lock requests are granted in order ----
[PASS] Lock requests are granted in order
---- Requests for distinct resources can be granted ----
[PASS] Requests for distinct resources can be granted
---- Returned Promise rejects if callback throws synchronously ----
[PASS] Returned Promise rejects if callback throws synchronously
---- Returned Promise rejects if async callback yields rejected promise ----
[PASS] Returned Promise rejects if async callback yields rejected promise
---- Locks are available once previous release is processed ----
[PASS] Locks are available once previous release is processed
---- callback must be a function ----
[PASS] callback must be a function
---- Lock attributes reflect requested properties (shared) ----
[PASS] Lock attributes reflect requested properties (shared)
---- navigator.locks.request's returned promise resolves after lock is released ----
[PASS] navigator.locks.request's returned promise resolves after lock is released
---- Returned Promise rejects if callback throws synchronously ----
[PASS] Returned Promise rejects if callback throws synchronously
---- Returned Promise rejects if callback throws asynchronously ----
[PASS] Returned Promise rejects if callback throws asynchronously
---- If callback throws a thenable, its then() should not be invoked ----
[PASS] If callback throws a thenable, its then() should not be invoked
---- query() returns dictionary with empty arrays when no locks are held ----
[PASS] query() returns dictionary with empty arrays when no locks are held
---- Lock requests are granted in order ----
[PASS] Lock requests are granted in order
---- Lock requests are granted in order ----
[PASS] Lock requests are granted in order
---- Releasing exclusive lock grants multiple shared locks ----
[UNEXPECTED_FAILURE][FAIL] Releasing exclusive lock grants multiple shared locks
assert_equals: An exclusive lock is held expected 1 but got 7
at Test.<anonymous> (/Users/debadreechatterjee/Documents/Personal/node/test/fixtures/wpt/web-locks/mode-mixed.tentative.https.any.js:59:3)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
Command: /Users/debadreechatterjee/Documents/Personal/node/out/Release/node /Users/debadreechatterjee/Documents/Personal/node/test/wpt/test-web-locks.js mode-mixed.tentative.https.any.js
---- Shared locks are not exclusive ----
[PASS] Shared locks are not exclusive
---- API presence in secure contexts ----
[PASS] API presence in secure contexts
---- An exclusive lock between shared locks ----
[UNEXPECTED_FAILURE][FAIL] An exclusive lock between shared locks
assert_equals: Shared locks are held expected 5 but got 6
at Test.<anonymous> (/Users/debadreechatterjee/Documents/Personal/node/test/fixtures/wpt/web-locks/mode-mixed.tentative.https.any.js:84:3)
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
Command: /Users/debadreechatterjee/Documents/Personal/node/out/Release/node /Users/debadreechatterjee/Documents/Personal/node/test/wpt/test-web-locks.js mode-mixed.tentative.https.any.js
{
"held.tentative.https.any.js": {
"skip": "Error: this uncaught rejection is expected"
},
"idlharness.tentative.https.any.js": {
"skip": "Unexpected global object"
},
"mode-mixed.tentative.https.any.js": {
"fail": {
"unexpected": [
"Releasing exclusive lock grants multiple shared locks",
"An exclusive lock between shared locks"
]
}
},
"non-secure-context.tentative.any.js": {
"skip": "navigator.locks is only present in secure contexts"
},
"query.tentative.https.any.js": {
"skip": "Uses Web Workers"
},
"resource-names.tentative.https.any.js": {
"skip": "Unexpected global object"
},
"signal.tentative.https.any.js": {
"skip": "Various errors. Process hangs."
},
"steal.tentative.https.any.js": {
"skip": "AbortError: The operation was aborted"
},
"storage-buckets.tentative.https.any.js": {
"skip": "Storage not in wpt"
}
}
Ran 8/16 tests, 8 skipped, 7 passed, 0 expected failures, 1 unexpected failures, 0 unexpected passes
/Users/debadreechatterjee/Documents/Personal/node/test/common/wpt.js:732
throw new Error(
^
Error: Found 1 unexpected failures. Consider updating test/wpt/status/web-locks.json for these files:
mode-mixed.tentative.https.any.js
at process.<anonymous> (/Users/debadreechatterjee/Documents/Personal/node/test/common/wpt.js:732:15)
at process.emit (node:events:523:35)
Node.js v20.0.0-pre
Command: out/Release/node /Users/debadreechatterjee/Documents/Personal/node/test/wpt/test-web-locks.js
[00:00|% 100|+ 0|- 1]: Done
Failed tests:
out/Release/node /Users/debadreechatterjee/Documents/Personal/node/test/wpt/test-web-locks.js |
This needs a rebase. @debadree25 if we want to open a new PR (make sure to mark Anna as a coauthor), I think that would be the easiest way to make this move forward. |
I'd certainly like to see this move forward but I think a separate PR is definitely the way to go unless @addaleax is interested in getting this branch updated. |
I will open a fresh one then, I have probably lost the fixes that I had done let me find that branch again! |
This is based upon #22719 and exposes the same API. Instead of a C++-backed approach, this variant implements the API mostly in JS.
Please feel free to push any changes that you think are reasonably uncontroversial to this branch yourself. 🙂 Tests are not going to pass until #36501 is merged.
/cc @devsnek @benjamingr @nodejs/workers
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes