node
node copied to clipboard
nodejs
src: add web locks api
This PR implements the Web Locks API, Locks are used to coordinate access to shared resources across multiple threads.
This implementation is based on previous work in https://github.com/nodejs/node/pull/22719 and https://github.com/nodejs/node/pull/36502, but takes a C++ native approach for better performance and reliability, this solution uses a singleton LockManager with thread-safe data structures to coordinate locks across all workers.
- [x] Support
exclusiveandsharedmodes - [x] Support
stealoption - [x] Support
ifAvailableoption - [x] Support
signaloption - [x] Documentation
- [x] WPT tests
- [x] Add missing
query.https.any.jstests as unit tests - [x] Add basic tests
Closes: #22702 Refs: https://w3c.github.io/web-locks
Review requested:
- [ ] @nodejs/gyp
- [ ] @nodejs/startup
- [ ] @nodejs/web-standards
Codecov Report
:x: Patch coverage is 79.20904% with 184 lines in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 90.02%. Comparing base (fc4a8af) to head (214a58e).
:warning: Report is 114 commits behind head on main.
| Files with missing lines | Patch % | Lines |
|---|---|---|
| src/node_locks.cc | 69.41% | 100 Missing and 67 partials :warning: |
| lib/internal/locks.js | 95.22% | 14 Missing :warning: |
| src/node_locks.h | 90.90% | 3 Missing :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #58666 +/- ##
==========================================
- Coverage 90.09% 90.02% -0.07%
==========================================
Files 645 648 +3
Lines 189930 190815 +885
Branches 37217 37392 +175
==========================================
+ Hits 171125 171790 +665
- Misses 11512 11674 +162
- Partials 7293 7351 +58
| Files with missing lines | Coverage Δ | |
|---|---|---|
| lib/internal/navigator.js | 99.37% <100.00%> (+0.04%) |
:arrow_up: |
| lib/worker_threads.js | 100.00% <100.00%> (ø) |
|
| src/node_binding.cc | 82.67% <ø> (ø) |
|
| src/node_external_reference.h | 100.00% <ø> (ø) |
|
| src/node_locks.h | 90.90% <90.90%> (ø) |
|
| lib/internal/locks.js | 95.22% <95.22%> (ø) |
|
| src/node_locks.cc | 69.41% <69.41%> (ø) |
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
![codecov[bot] avatar](https://avatars.githubusercontent.com/in/254?v=4 "Published by a pub.dev verified publisher"){kind=small}
Adding https://github.com/nodejs/node/labels/semver-major as this adds new globals (Lock, LockManager)
There seems to be a related test failure on linuxone. I didn't look closer into the code, so I can't tell why: https://ci.nodejs.org/job/node-test-commit-linuxone/50100/
To avoid semver-major and allow early testing, this could be exposed from a builtin module and only added as a global later.
@targos I remove it from globals, could you please replace semver-major by semver-minor
The CI test failures are not flaky failures. Before running the CI again on this the relevant failures should be looked at
Failed to start CI
⚠ Commits were pushed since the last approving review: ⚠ - worker: add web locks api ⚠ - use WebIDL convertors ⚠ - add lock and lockmanager to globals doc ⚠ - remove lock and lockmanager from globals ⚠ - attemp to atach catch on c++ by defering to next microtask ⚠ - separate promise fulfillment and rejection handlers ⚠ - add cjs/esm code in locks doc ⚠ - add more tests and doc ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/15825543437
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
I think this is very close! Added a few comments and notice that there are WPT test failures.
@jasnell I fixed all your comments in the last commit, can you take a look and rerun the CI? FYI a couple of WPT tests were flaky on linuxone RHEL 8/9 machines, I cleaned them all up - let’s see what the CI says 🤞
The https://github.com/nodejs/node/labels/notable-change label has been added by @anonrig.
Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.
I still think there may be at least a few issues here relating to how you're using v8::Global and v8::External. I'd really like to see if we could get @addaleax to take a look.
(looks like CI failed because of a merge conflict? might just need to rebase this 🙂)
