Skip to content

WIP: Web Locks API initial implementation #417

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
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

WIP: Web Locks API initial implementation #417

wants to merge 3 commits into from

Conversation

@tshemsedinov
Copy link
Member

@tshemsedinov tshemsedinov commented Mar 26, 2019

Refs: #416

@belochub
Copy link
Member

belochub commented Mar 27, 2019

@tshemsedinov isn't it supposed to be compliant with the spec for Web Locks API? It defines two interfaces: LockManager and Lock, both of which are not present in the implementation in this PR. Thus I don't think it should be called 'Web Locks API implementation' unless you implement the interfaces mentioned above.

@tshemsedinov
Copy link
Member Author

tshemsedinov commented Mar 27, 2019

Yes, but it's just proof of concept and WIP. @belochub I'll implement also
options: { mode: "exclusive" | "shared", signal <AbortSignal>, ifAvailable <boolean> } and extend specification with timeout, recursive locking and locking queue in each thread, error handling in critical section, etc.

@tshemsedinov tshemsedinov self-assigned this Mar 28, 2019
lundibundi
lundibundi reviewed Apr 8, 2019
View reviewed changes
lib/locks.js
const prev = Atomics.exchange(this.flag, 0, LOCKED);
if (prev === LOCKED) return;
this.owner = true;
this.trying = false;
Copy link
Member

@lundibundi lundibundi Apr 8, 2019

Choose a reason for hiding this comment

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

Is this.trying needed?

Copy link
Member Author

@tshemsedinov tshemsedinov Apr 8, 2019

Choose a reason for hiding this comment

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

We can use mutex.queue.length > 0 instead

lib/locks.js
return this.tryEnter();
}

tryEnter() {
Copy link
Member

@lundibundi lundibundi Apr 8, 2019

Choose a reason for hiding this comment

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

Perhaps we can merge tryEnter and enterIfAvailable by adding lock argument and a few checks?

lib/locks.js Outdated
enterIfAvailable(lock) {
if (this.owner) return lock.callback();
const prev = Atomics.exchange(this.flag, 0, LOCKED);
if (prev === LOCKED) return lock.callback();
Copy link
Member

@lundibundi lundibundi Apr 8, 2019

Choose a reason for hiding this comment

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

According to https://wicg.github.io/web-locks/#api-lock-manager-request when ifAvailable is true the callback must be passed a lock (boolean) argument that signifies whether it got the lock or not.

lundibundi
lundibundi reviewed Apr 8, 2019
View reviewed changes
@tshemsedinov tshemsedinov force-pushed the web-losks-api branch 4 times, most recently from 465e50b to 18bcd04 Compare April 8, 2019 22:17
@tshemsedinov
Copy link
Member Author

tshemsedinov commented Dec 1, 2019

In new implementation:

  • Mutex class is merged into Lock class
  • Added: AbortError, AbortSignal, AbortController

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lundibundi lundibundi lundibundi left review comments

@nechaido nechaido Awaiting requested review from nechaido

@belochub belochub Awaiting requested review from belochub

@flaminisx flaminisx Awaiting requested review from flaminisx

Assignees

@tshemsedinov tshemsedinov

Labels

functionality research

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tshemsedinov @belochub @lundibundi