Add Component Locking

[lmars]

This is a draft PR to get feedback on an early implementation of Component Locking.

This is not a complete implementation, but opening for early feedback and to align with changes proposed in #101.

[UPDATE: 2023-08-07]

I have now implemented all four proposed locking features: Query, Acquire, Release, and Subscribe.

Query

space.locks.get is used to query whether a lock identifier is currently locked and by whom. It returns a Lock type which has the following fields:

type Lock = {
  member: SpaceMember;
  request: LockRequest;
};

For example:

// check if the id is locked
const isLocked = space.locks.get(id) !== undefined;

// check which member has the lock
const { member } = space.locks.get(id);

// check the lock attributes assigned by the member holding the lock
const { request } = space.locks.get(id);
const value = request.attributes.get(key);

Acquire

space.locks.acquire initialises a lock request, adds it to the locks field of presence message extras, and calls presence.update.

It returns a Promise which resolves once presence.update resolves.

const req = await space.locks.acquire(id);

// or with some attributes
const attributes = new Map();
attributes.set('key', 'value');
const req = await space.locks.acquire(id, { attributes });

It throws an error if a lock request already exists for the given identifier with a status of PENDING or LOCKED.

Release

space.locks.release releases a previously requested lock by removing it from the locks field of presence message extras, and calls presence.update.

It returns a Promise which resolves once presence.update resolves.

await space.locks.release(id);

Subscribe

space.locks.subscribe subscribes to changes in lock status across all members.

The callback is called with a value of type Lock.

space.locks.subscribe('update', (lock) => {
  // lock.member is the requesting member
  // lock.request is the request made by the member
});

// or with destructuring:
space.locks.subscribe('update', ({ member, request }) => {
  // request.status is the status of the request, one of PENDING, LOCKED, or UNLOCKED
  // request.reason is an ErrorInfo if the status is UNLOCKED
});

Such changes occur when:

• a PENDING request transitions to LOCKED (i.e. the requesting member now holds the lock)
• a PENDING request transitions to UNLOCKED (i.e. the requesting member does not hold the lock since another member already does)
• a LOCKED request transitions to UNLOCKED (i.e. the lock was either released or invalidated by a concurrent request which took precedence)
• an UNLOCKED request transitions to LOCKED (i.e. the requesting member reacquired a lock)

---

I've also added an example in examples/locks.ts which can be run with npm run examples:locks:

$ npm run examples:locks

> @ably-labs/spaces@0.0.12 examples:locks
> ts-node ./examples/locks.ts

2023-08-07T20:26:16.220Z INFO initialising Ably client
2023-08-07T20:26:16.222Z INFO entering the "example" space
2023-08-07T20:26:16.670Z INFO setting location to {"slideId":"123","elementId":"456"}
2023-08-07T20:26:16.976Z INFO checking if location is locked
2023-08-07T20:26:16.978Z INFO location is not locked
2023-08-07T20:26:16.978Z INFO subscribing to lock events
2023-08-07T20:26:16.978Z INFO attempting to lock "/slides/123/element/456"
2023-08-07T20:26:17.012Z INFO waiting for lock event
2023-08-07T20:26:17.017Z INFO received lock update for "/slides/123/element/456", status=locked
2023-08-07T20:26:17.018Z INFO unsubscribing from lock events
2023-08-07T20:26:17.018Z INFO lock status is "locked"
2023-08-07T20:26:17.018Z INFO releasing the lock
2023-08-07T20:26:17.514Z INFO done

---

MMB-213 [MMB-214] [MMB-215]

[lmars]

I have now implemented the four proposed locking features: Query, Acquire, Release, and Subscribe (see the PR description).

I've also added an example of locking locations in examples/lock.ts (I wanted to add this to the demo, but I was finding that difficult).

This is not yet ready for merge since it requires an ably-js release including ably/ably-js#1418, but it is otherwise ready for review.

cc @dpiatek @paddybyers @mattheworiordan

[Srushtika]

Thanks @lmars , a few questions/ comments:

1. In Query, const value = request.attributes.get(key); - does this mean we'll expose a get method for the attributes as well? How is that different from request.attributes.key or request.attributes[key]?
2. With the release() method, one can only ever release a lock they acquired because it's part of their presence data. Due to this there won't be any case of accidentally releasing someone else's lock right? I know that's not a use-case we are supporting right now but just wanted to confirm that's the case.
3. For Subscribe, given that we don't send a message when a lock enters the pending state from scratch (following someone requesting it), what's the thinking behind including this event: a PENDING request transitions to UNLOCKED (i.e. the requesting member does not hold the lock since another member already does)? Who is it meant for and what is the use-case?
4. re an UNLOCKED request transitions to LOCKED (i.e. the requesting member reacquired a lock) - if they require, will it not go into the pending state again triggering the first subscription event?

[lmars]

@Srushtika

In Query, const value = request.attributes.get(key); - does this mean we'll expose a get method for the attributes as well? How is that different from request.attributes.key or request.attributes[key]?

I decided to explicitly type attributes as a Map<string, string> rather than being an arbitrary object for future compatibility with the potential to interpret them server-side, which knowing the structure helps with. Do you think that might be problematic?

With the release() method, one can only ever release a lock they acquired because it's part of their presence data. Due to this there won't be any case of accidentally releasing someone else's lock right? I know that's not a use-case we are supporting right now but just wanted to confirm that's the case.

Correct, release will only affect the current member's locks.

For Subscribe, given that we don't send a message when a lock enters the pending state from scratch (following someone requesting it), what's the thinking behind including this event: a PENDING request transitions to UNLOCKED (i.e. the requesting member does not hold the lock since another member already does)? Who is it meant for and what is the use-case?

When a member calls acquire, the request is first in the PENDING state, and then the member needs to wait for that request to be added to presence and processed (i.e. compared to lock requests for the same ID from other members) before knowing whether they got the lock or not, which is relayed to them by emitting either a LOCKED or UNLOCKED event.

Typical usage is like:

const req = space.locks.acquire(id);
// req.status is PENDING

// wait for a status change
const status = await new Promise<LockStatus>(resolve => {
  space.locks.subscribe('update', lock => {
    if (lock.request.id == req.id) {
      resolve(lock.request.status);
    }
  });
});

if (status == LockStatus.LOCKED) {
  // member got the lock
} else {
  // member did not get the lock
}

re an UNLOCKED request transitions to LOCKED (i.e. the requesting member reacquired a lock) - if they require, will it not go into the pending state again triggering the first subscription event?

It will, but we don't emit an event for a lock becoming PENDING, only when a lock either becomes LOCKED or UNLOCKED. This is essentially to be future compatible with a server-side implementation where a member will never see PENDING lock requests from other members, since the server will always intercept those PENDING requests and transition them to LOCKED or UNLOCKED before broadcasting to other members, at which point other members might see a lock go straight from UNLOCKED to LOCKED without seeing the intermediate PENDING status.

[Srushtika]

before knowing whether they got the lock or not, which is relayed to them by emitting either a LOCKED or UNLOCKED event.

Yes, but why do others need to know about someone's failed attempt to lock a component through subscriptions?

[dpiatek]

This encompasses the foundation of what we discussed in our work. I've left a couple of small comments, questions, but I think this is a great first iteration of the API.

[lmars]

@dpiatek I've addressed your comments, PTAL

[dpiatek]

@lmars two small comments otherwise LGTM

[lmars]

@mattheworiordan I had requested your review but I've gone ahead and merged this, please feel free to leave comments after the fact though and I can follow up in future PRs.

[mattheworiordan]

I had requested your review but I've gone ahead and merged this, please feel free to leave comments after the fact though and I can follow up in future PRs.

Yup, sorry, I've not had time. I will add comments in a separate post or inline shortly.

[mattheworiordan]

When should we start adding these error codes to ably-common. My concern is if we leave it to the end, we may forget / have codes scattered across SDKs. Should we perhaps consider doing this when we merge things to main?

[lmars]

I think they should be added alongside the PR yes, which I didn't do here, but I did ticket it as a priority for next sprint: https://ably.atlassian.net/browse/MMB-238.

I think we might want to reserve a range specifically for Spaces, like we'd done for Asset Tracking.

[mattheworiordan]

What timestamp format are we planning on supporting when this goes server-side? Would it be a JS timestamp value and we'll assume all other languages use that?

[paddybyers]

It's going to be a millisecond timestamp, at least, and might at some point extend to being a timeserial

[lmars]

Yeh, millisecond unix timestamp, I'll document this in the feature spec next sprint (https://ably.atlassian.net/browse/MMB-235).

[mattheworiordan]

Shouldn't this be private? Not sure on support for this, but I thought the recommended approach is # prefix?

[lmars]

It's called by the Space class here, so it can't be private no.

[mattheworiordan]

Thanks for this @lmars, great start and simple which is nice.

This is not a complete implementation, but opening for early feedback

I am finding it hard what to comment on or not in terms of functionality because you're not saying which parts of the implementation you have omitted, vs decided not to implement. This makes it hard for me to comment on what's not here as a result. On the call, for example, I mentioned you had not included a way to iterate over the locks, and you said you'd add that. Had you decided not to include that method, or just not implemented it yet? Given this, I feel it would be a waste of time to comment on what's not here, and would prefer to do that once you present an IDL of a complete API. Would it make sense to have that evolve as you do PRs, so we can comment on what you're removing/adding/changing to the complete API as well?

It returns a Lock type which has the following fields:
type Lock = { member: SpaceMember; request: LockRequest; };

Referring to Paddy's proposal, he stated:

By allowing userdata to be provided as part of a lock entry, this enables the feature to be used as a convenient primitive for other higher-level services to be constructed client-side (eg leader election, state synchronisation). (These kinds of use-cases are described in the Chubby paper).

Do you envisage userData is just part of attributes for the lock, and immutable whilst the lock is held? I assume so, but just checking.

It returns a Lock type which has the following fields:
type Lock = { member: SpaceMember; request: LockRequest; };

I am not mad about the attribute request for LockRequest. The lock has an ID, a status, attributes, not the request. I don't think the naming / modelling is valid from a user's perspective, and instead we're using the name request simply because a request is made to obtain the lock, but once locked, the naming request does not make much sense. Why aren't the LockRequest attributes set at the root level of the Lock object?

In this code example:

const isLocked = space.locks.get(lockId) !== undefined;
  if (isLocked) {
    info('location is already locked');
    process.exit();
  }

I have two comments/questions:

• Calling if (space.locks.get(lockId)) { ..handle can't obtaining lock.. }} if the user happens to have the lock already, will end up calling the block. I think we should be providing a convenience that provides filters for my locks, or all locks by my locks. This could be done with chaining such as space.locks.mine.get(lockId) or arguments, I'm unopinionated on that. However I believe the pattern will be so common it would be odd for us to not offer that. I see competing APIs do something similar. We have done something similar in members with getSelf.
• I realise get is just a proxy to finding out if isLocked, but I feel like we could do better for readability with something like:

  if (space.locks.isLocked(lockId, exclude_self: true) {
    info('location is already locked');
    process.exit();
  }

is just nicer to read and more intuitive.

⚠️ Please note I see I missed some of the replies to my comments on https://ably.atlassian.net/wiki/spaces/product/pages/2668298321/MMDR19+Component+Locking+API, I've since replied to them all. Note that some apply to this API.

❓ Should we close https://github.com/ably/ideas/issues/459 now that this work supersedes it?

[lmars]

@mattheworiordan

I am finding it hard what to comment on or not in terms of functionality because you're not saying which parts of the implementation you have omitted, vs decided not to implement

Apologies, that initial description was when this PR was a draft for early feedback, I subsequently implemented all functionality from our internal API design doc (i.e. Query, Acquire, Release, and Subscribe), which this now covers. If there is anything missing from the design doc, please point it out, but I realise I omitted getAll, which I'll follow up with.

Given this, I feel it would be a waste of time to comment on what's not here, and would prefer to do that once you present an IDL of a complete API

Can you clarify what you mean by IDL? Something beyond what is in the API design doc? I wasn't planning to write anything beyond that.

Do you envisage userData is just part of attributes for the lock, and immutable whilst the lock is held? I assume so, but just checking.

Yes, attributes are the equivalent of userData from that proposal. There is currently no API to modify the attributes, but it isn't enforced, there's nothing stopping a member updating the attributes directly in presence data, and that being broadcast to all members. Do you think there is a requirement that they should be immutable?

Why aren't the LockRequest attributes set at the root level of the Lock object?

Yes we could have Lock extend LockRequest, I'll consider that in a future PR.

I think we should be providing a convenience that provides filters for my locks, or all locks by my locks ... I feel like we could do better for readability with something like

Roger that, I'll write something up specific to enhancing the Query implementation rather than discussing here.

❓ Should we close https://github.com/ably/ideas/issues/459 now that this work supersedes it?

I'll do that once I've written the server-side implementation plan (https://ably.atlassian.net/browse/MMB-197).

[mattheworiordan]

Can you clarify what you mean by IDL? Something beyond what is in the API design doc? I wasn't planning to write anything beyond that.

See the "Interface Definition" of the core SDK spec or the TypeScript definition of ably-js. In one place, we can see the API surface for an SDK, and when reviewing individual suggestions around what an API should look like, we can see how that fits with the other APIs. There is no practical way as we working for anyone to see how the overall API surface is shaping up and reflect on how consistent and/or intuitive it is. I realise your work is just in locking, so admittedly this is not your problem. However, I do believe it's a problem more broadly for this SDK, and any other product SDKs we develop, when we don't have a single TypeScript definition / or IDL-like definition we can look at in totality. What are your thoughts on this?

@stmoreau and @Srushtika WDYT, coming from the perspective of how we operationalise the maintenance and development of SDKs more generally?

There is currently no API to modify the attributes, but it isn't enforced, there's nothing stopping a member updating the attributes directly in presence data, and that being broadcast to all members. Do you think there is a requirement that they should be immutable?

Perhaps one for @paddybyers to comment on given he was the one who suggested it. My sense is we should either reject changes once a lock is obtained, or provide an API to actually make and subscribe to lock attribute changes.

[paddybyers]

There is currently no API to modify the attributes, but it isn't enforced, there's nothing stopping a member updating the attributes directly in presence data, and that being broadcast to all members. Do you think there is a requirement that they should be immutable?

Perhaps one for @paddybyers to comment on given he was the one who suggested it. My sense is we should either reject changes once a lock is obtained, or provide an API to actually make and subscribe to lock attribute changes.

I think the main aim of having the userdata is that participants can rely on the state set there being globally authoritative for the channel. If it can change, then that just detracts from that - to be useful, there would need then to be explicit versioning, events when it changes, etc. So I think, at least for now, we should say that it's immutable. We could consider it being mutable but that would require us thinking about what API a user would really need around that