Skip to content
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

WIP: Web Locks API initial implementation #417

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

Refs: #416

@belochub
Copy link
Member

@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

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 Outdated Show resolved Hide resolved
lib/locks.js Outdated Show resolved Hide resolved
lib/locks.js
const prev = Atomics.exchange(this.flag, 0, LOCKED);
if (prev === LOCKED) return;
this.owner = true;
this.trying = false;
Copy link
Member

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

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

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

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.

@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

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.
3 participants
@tshemsedinov @belochub @lundibundi