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.

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

IlyasShabi
Copy link
Contributor

@IlyasShabi IlyasShabi commented Jun 10, 2025

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

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
Copy link

codecov bot commented Jun 10, 2025

Codecov Report

Attention: Patch coverage is 85.87786% with 111 lines in your changes missing coverage. Please review.

Project coverage is 90.09%. Comparing base (462c741) to head (9ee6d05).
Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
src/node_locks.cc 78.68% 52 Missing and 45 partials ⚠️
lib/internal/locks.js 95.22% 14 Missing ⚠️
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     
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 100.00% <100.00%> (ø)
lib/internal/locks.js 95.22% <95.22%> (ø)
src/node_locks.cc 78.68% <78.68%> (ø)

... and 60 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
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

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 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
@panva
Copy link
Member

panva commented Jun 17, 2025

@IlyasShabi can you rebase so that we can start CI?

@panva panva added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jun 17, 2025
@IlyasShabi
Copy link
Contributor Author

@panva can you run the CI again?

@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
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@BridgeAR
Copy link
Member

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/

@targos
Copy link
Member

targos commented Jun 18, 2025

To avoid semver-major and allow early testing, this could be exposed from a builtin module and only added as a global later.

@IlyasShabi
Copy link
Contributor Author

@targos I remove it from globals, could you please replace semver-major by semver-minor

@targos targos added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Jun 18, 2025
@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 19, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 19, 2025
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@jasnell
Copy link
Member

jasnell commented Jun 20, 2025

The CI test failures are not flaky failures. Before running the CI again on this the relevant failures should be looked at

@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. 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
Development

Successfully merging this pull request may close these issues.