Skip to content

fix: keep a resolved lock when a later presence message lists it as pending - #366

Open
RaphaelFakhri wants to merge 1 commit into
ably:mainfrom
RaphaelFakhri:fix/locks-concurrent-acquire-pending-reset
Open

RaphaelFakhri wants to merge 1 commit into
ably:mainfrom
RaphaelFakhri:fix/locks-concurrent-acquire-pending-reset

Conversation

@RaphaelFakhri

Copy link
Copy Markdown

Related to #338

Description

When a member acquires several locks concurrently, for example with Promise.all(), acquire() publishes one presence update per lock. Each update carries every lock request the member has so far, and none of them are resolved yet, so the later messages list the earlier locks as pending again.

Locks.processPresenceMessage resolves a pending request to locked (or unlocked) when it first sees it. When the next message arrived, the same request was still listed as pending, so the stored lock moved back from locked to pending. The update event reported pending, and no later event reported locked again. Depending on message order, only some of the locks stayed locked, which matches the inconsistent results in #338.

This change ignores a pending entry when the stored request for the same lock has the same timestamp and is already resolved.

Testing

  • Adds a Locks test that processes two presence messages in the order acquire() publishes them: [a pending], then [a pending, b pending]. It expects both locks to be locked.
  • The test fails without the change (expected 'pending' to be 'locked', 1 failed of 21) and passes with it (21 passed).
  • The full unit suite passes (119 tests). prettier --check and eslint are clean.

Run with:

npx vitest run src/Locks.test.ts

@ttypic ttypic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks for contributing

This branch has not been deployed

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants