Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Closed
IlyasShabi wants to merge 17 commits into from
Closed

src: add web locks api #58666

IlyasShabi wants to merge 17 commits into from

Conversation

@IlyasShabi
Copy link
Member

@IlyasShabi IlyasShabi commented Jun 10, 2025 •
edited
Loading

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.

  • Support exclusive and shared modes
  • Support steal option
  • Support ifAvailable option
  • Support signal option
  • Documentation
  • WPT tests
  • Add missing query.https.any.js tests as unit tests
  • Add basic tests

Closes: #22702
Refs: https://w3c.github.io/web-locks

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 10, 2025

Review requested:

  • @nodejs/gyp
  • @nodejs/startup
  • @nodejs/web-standards

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jun 10, 2025
@IlyasShabi IlyasShabi changed the title Add web locks api src: add web locks api Jun 10, 2025
@IlyasShabi IlyasShabi marked this pull request as ready for review June 10, 2025 19:47
@codecov
Copy link

codecov bot commented Jun 10, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 79.20904% with 184 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.02%. Comparing base (fc4a8af) to head (214a58e).
⚠️ 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 ⚠️
lib/internal/locks.js 95.22% 14 Missing ⚠️
src/node_locks.h 90.90% 3 Missing ⚠️
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%) ⬆️
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%> (ø)

... and 42 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jasnell jasnell requested a review from addaleax June 10, 2025 21:00
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 10, 2025
jasnell
jasnell reviewed Jun 10, 2025
View reviewed changes
panva
panva reviewed Jun 11, 2025
View reviewed changes
atlowChemi
atlowChemi reviewed Jun 11, 2025
View reviewed changes
mcollina
mcollina approved these changes Jun 14, 2025
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Ethan-Arrowood
Ethan-Arrowood approved these changes Jun 16, 2025
View reviewed changes
Copy link
Contributor

@Ethan-Arrowood Ethan-Arrowood left a 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!

@panva panva added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Jun 16, 2025
@panva
Copy link
Member

panva commented Jun 16, 2025

Adding semver-major PRs that contain breaking changes and should be released in the next major version. as this adds new globals (Lock, LockManager)

@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 16, 2025
panva
panva reviewed Jun 16, 2025
View reviewed changes
@panva panva added web-standards Issues and PRs related to Web APIs and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jun 16, 2025
@IlyasShabi IlyasShabi requested a review from panva June 17, 2025 09:53
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 17, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 17, 2025
@joyeecheung
Copy link
Member

joyeecheung commented Jul 21, 2025

wpt/test-web-locks has been flaky and failed at least 5 PRs out of the last 100 CI runs: #59142

@jasnell
Copy link
Member

jasnell commented Jul 21, 2025

Yeah, been noticing that. Let's mark it flaky. @IlyasShabi do you think you'd be able to investigate?

@IlyasShabi
Copy link
Member Author

IlyasShabi commented Jul 21, 2025

@jasnell I have a PR to mark it flaky #59144. I will do investigate this later

@github-actions github-actions bot mentioned this pull request Jul 28, 2025
Merged
@aduh95 aduh95 mentioned this pull request Jul 29, 2025
Merged
@aduh95
Copy link
Contributor

aduh95 commented Aug 4, 2025

This doesn't land cleanly on v22.x-staging, we would need a manual backport PR if we want it on v22.x.

@aduh95 aduh95 added backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 4, 2025
@RafaelGSS RafaelGSS mentioned this pull request Aug 12, 2025
Merged
@aduh95 aduh95 mentioned this pull request Aug 20, 2025
Merged
This was referenced Oct 30, 2025
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@atlowChemi atlowChemi atlowChemi left review comments

@mcollina mcollina mcollina approved these changes

@panva panva panva approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@benjamingr benjamingr benjamingr approved these changes

@anonrig anonrig anonrig approved these changes

@Ethan-Arrowood Ethan-Arrowood Ethan-Arrowood approved these changes

@marco-ippolito marco-ippolito marco-ippolito approved these changes

@KhafraDev KhafraDev Awaiting requested review from KhafraDev

Assignees

No one assigned

Labels

backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. web-standards Issues and PRs related to Web APIs

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.