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 13 commits into
base: main
Choose a base branch
from
Open

src: add web locks api #58666

wants to merge 13 commits into from

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 79.09091% with 184 lines in your changes missing coverage. Please review.

Project coverage is 90.04%. Comparing base (462c741) to head (8ba4c08).
Report is 81 commits behind head on main.

Files with missing lines Patch % Lines
src/node_locks.cc 68.92% 100 Missing and 70 partials ⚠️
lib/internal/locks.js 95.22% 14 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58666      +/-   ##
==========================================
- Coverage   90.15%   90.04%   -0.12%     
==========================================
  Files         639      643       +4     
  Lines      188201   189311    +1110     
  Branches    36915    37139     +224     
==========================================
+ Hits       169675   170463     +788     
- Misses      11274    11506     +232     
- Partials     7252     7342      +90     
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 68.92% <68.92%> (ø)

... and 89 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
@IlyasShabi
Copy link
Contributor Author

IlyasShabi commented Jun 25, 2025

@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 🤞

@anonrig anonrig added notable-change PRs with changes that should be highlighted in changelogs. request-ci Add this label to start a Jenkins CI on a PR. labels Jun 26, 2025
Copy link
Contributor

The notable-change PRs with changes that should be highlighted in changelogs. 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.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2025
@nodejs-github-bot
Copy link
Collaborator

@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2025
@nodejs-github-bot
Copy link
Collaborator

Local<Promise> promise = callback_result.As<Promise>();
if (promise->Then(context, on_fulfilled_callback, on_rejected_callback)
.IsEmpty()) {
// Then() failed, reject both promises and return
Copy link
Member

Choose a reason for hiding this comment

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

If Then returns an empty, then v8 will have already scheduled an error. You can probably use a v8::TryCatch in here and reject the waiting_promise() and released_promise() with it's Exception(), but you'd want to check CanContinue() first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Just putting the red x on this since CI passed and it has sufficient sign off from others to land but there are still a few outstanding issues to resolve. I want to make sure it doesn't end up getting landed by the commit-queue while some comments are still pending.

@IlyasShabi IlyasShabi requested a review from jasnell June 30, 2025 08:00
@nodejs-github-bot
Copy link
Collaborator

// Called when the promise returned from the user's callback rejects
static void OnIfAvailableReject(const FunctionCallbackInfo<Value>& info) {
HandleScope handle_scope(info.GetIsolate());
auto* holder = static_cast<Global<Promise::Resolver>*>(
Copy link
Member

Choose a reason for hiding this comment

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

These's aren't correct here. You really should not be holding a v8::Global<T> inside a v8::External, then deleting it like this. It's fine to use indirection through another type... e.g. having the External hold an instance of a struct that is holding the Global<T>.

@jasnell
Copy link
Member

jasnell commented Jun 30, 2025

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.

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. 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
Development

Successfully merging this pull request may close these issues.