-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
src: add web locks api #58666
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
base: main
Are you sure you want to change the base?
src: add web locks api #58666
Conversation
Review requested:
|
370462f
to
ba53a01
Compare
ba53a01
to
5d52680
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #58666 +/- ##
==========================================
- Coverage 90.15% 90.09% -0.07%
==========================================
Files 639 643 +4
Lines 188201 189085 +884
Branches 36915 37077 +162
==========================================
+ Hits 169675 170355 +680
- Misses 11274 11420 +146
- Partials 7252 7310 +58
🚀 New features to boost your workflow:
|
5d52680
to
6fdd577
Compare
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.
lgtm
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 lgtm. I looked through the WPT and other tests and they look all good. I reviewed the JS api to make sure it aligns with expectations and analyzed its source. All is good for a first implementation. I did a cursory review of the C++ part as I'm less experienced there, but in general it looks okay too. Nice work!
Adding
semver-major
|
@IlyasShabi can you rebase so that we can start CI? |
ed3a96a
to
a8810fc
Compare
@panva can you run the CI again? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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 |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
The CI test failures are not flaky failures. Before running the CI again on this the relevant failures should be looked at |
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 #22719 and #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.exclusive
andshared
modessteal
optionifAvailable
optionsignal
optionquery.https.any.js
tests as unit testsCloses: #22702
Refs: https://w3c.github.io/web-locks