Skip to content

feat: add AsyncLock and split helpers into modules - #90

Merged
mykola-mokhnach merged 2 commits into
masterfrom
lock
Oct 23, 2026
Merged

mykola-mokhnach merged 2 commits into
masterfrom
lock

Conversation

@mykola-mokhnach

@mykola-mokhnach mykola-mokhnach commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Vendor a promise-only, TypeScript rewrite of the unmaintained async-lock package as AsyncLock:

  • tasks run in the AsyncLocalStorage context of their own acquire caller instead of the previous lock holder's one
  • a task outliving maxExecutionTime can no longer release the lock a second time
  • queued tasks that time out are removed from the queue
  • getQueueLength(key) exposes the number of waiting tasks
  • a multi-key acquire holds its keys together, claims them in sorted order (no deadlocks between overlapping calls) and releases them all before starting any waiter
  • callbacks, domain reentrancy and custom Promise support are dropped

Also split lib/asyncbox.ts into one module per helper with explicit named re-exports from lib/index.ts, mirror that layout in the specs, and replace wall-clock assertions in the tests with deterministic ones to fix flakiness. longSleep and waitForCondition now measure intervals with performance.now() and report whole milliseconds.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The multi-key occupation-time limit has an unresolved moderate issue, and several timing tests still depend on wall-clock behavior.

3 open issues
What changed in this PR

This pull request introduces a promise-based AsyncLock, modularizes the helper library, and updates package exports, documentation, and tests.

Changes:

  • Adds keyed locking with queue limits, timeouts, execution limits, async-context preservation, and queue inspection.
  • Splits lib/asyncbox.ts into focused helper modules with explicit exports from lib/index.ts.
  • Reorganizes tests and adds coverage for locking, public exports, CommonJS interoperability, and concurrency behavior.
File Description
test/​with-timeout.spec.ts Tests timeout behavior and custom errors.
test/​wait-for-condition.spec.ts Tests condition polling and timeout behavior; one success case still relies on wall-clock timing.
test/​sleep.spec.ts Tests cancellable sleep behavior and options; one assertion still relies on elapsed real time.
test/​retry.spec.ts Tests retry and retry-interval behavior.
test/​long-sleep.spec.ts Tests long-sleep behavior and progress callbacks; duration checks still rely on real wall-clock timing.
test/​lock.spec.ts Covers locking, queuing, timeouts, execution limits, and async-context behavior.
test/​index.spec.ts Verifies the documented public runtime exports.
test/​helpers.ts Provides timer-recording and concurrency-tracking test helpers.
test/​cjs-interop.spec.cjs Verifies CommonJS access to AsyncLock.
test/​asyncmap.spec.ts Tests the extracted asynchronous mapping helper.
test/​asyncfilter.spec.ts Tests the extracted asynchronous filtering helper.
test/​asyncbox.spec.ts Removes the former monolithic helper test suite.
README.md Documents AsyncLock and its options; the meaning of maxPending: 0 is ambiguous.
package.json Updates package entry points and type declarations to the new index module.
lib/​with-timeout.ts Implements the extracted promise timeout helper.
lib/​wait-for-condition.ts Implements the extracted condition-waiting helper.
lib/​sleep.ts Implements cancellable sleeping.
lib/​retry.ts Implements retry and interval-retry helpers.
lib/​long-sleep.ts Implements long-duration sleeping with progress callbacks.
lib/​lock.ts Implements the keyed AsyncLock; multi-key acquisition does not share one maxOccupationTime deadline across all keys.
lib/​limit-function.ts Lazily loads p-limit for concurrency control.
lib/​index.ts Defines explicit public runtime and type exports.
lib/​errors.ts Defines exported timeout and cancellation errors.
lib/​asyncmap.ts Implements asynchronous mapping with concurrency support.
lib/​asyncfilter.ts Implements asynchronous filtering with concurrency support.
lib/​asyncbox.ts Removes the former combined helper implementation.

Review effort: Lite

Comment thread lib/lock.ts Outdated
Comment thread test/sleep.spec.ts Outdated
Comment thread test/wait-for-condition.spec.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Address the critical AsyncLock key validation issue, correct the expired multi-key timeout handling, and stabilize the remaining wall-clock-dependent test.

1 open issue
3 resolved since last review

Review effort: Lite

Comment thread lib/lock.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved lock API and multi-key timeout issues, including one critical correctness issue, block approval.

2 open issues
1 resolved since last review

Review effort: Lite

Comment thread lib/lock.ts Outdated
Comment thread lib/lock.ts
@eglitise
eglitise requested a balanced review from Copilot October 22, 2026 16:04

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

A critical multi-key release-ordering issue remains unresolved; the README wording also needs clarification.

1 open issue
2 resolved since last review

Review effort: Lite

Comment thread lib/lock.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

Unresolved critical deadlock and moderate correctness issues remain in lib/lock.ts and lib/long-sleep.ts.

1 open issue
1 resolved since last review

Review effort: Lite

Comment thread lib/lock.ts Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔵 Needs a closer look

Two moderate issues in lib/lock.ts remain unresolved: timeout messaging for executing tasks and validation of sparse key arrays.

No open issues 🎉

1 resolved since last review

Review effort: Lite

Vendor a promise-only, TypeScript rewrite of the unmaintained
async-lock package as `AsyncLock`:

- tasks run in the AsyncLocalStorage context of their own `acquire`
  caller instead of the previous lock holder's one
- a task outliving `maxExecutionTime` can no longer release the lock
  a second time
- queued tasks that time out are removed from the queue
- `getQueueLength(key)` exposes the number of waiting tasks
- a multi-key `acquire` holds its keys together, claims them in sorted
  order (no deadlocks between overlapping calls) and releases them all
  before starting any waiter
- callbacks, domain reentrancy and custom Promise support are dropped

Also split lib/asyncbox.ts into one module per helper with explicit
named re-exports from lib/index.ts, mirror that layout in the specs,
and replace wall-clock assertions in the tests with deterministic ones
to fix flakiness. `longSleep` and `waitForCondition` now measure
intervals with `performance.now()` and report whole milliseconds.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Comment thread lib/lock.ts Outdated
this.queues,
this.keys[index],
{timeout, maxPending, skipQueue},
() => this.claimKey(index + 1),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] Avoid synchronous recursion when claiming a large batch of keys

Each free key synchronously calls claimKey again through join() → grant() → onGranted(), so a large valid key array can exhaust the call stack before the task starts. On Node 24, I reproduced this with 2,000 distinct keys: acquire() throws RangeError: Maximum call stack size exceeded, and the keys already claimed remain locked. If the batch initially queues behind another holder, the same overflow during handover instead produces an unhandled rejection while the batch's public promise remains pending. With the default unlimited timeouts, subsequent acquisitions of those keys can wait indefinitely.

Could we advance through immediately available keys iteratively and ensure acquisition failures release existing claims? Regression tests should cover both an immediately available large batch and one resumed after waiting for its first key.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 9c039c8. Free keys are now claimed in a loop instead of recursing through join() → grant() → onGranted(), so the stack no longer grows with the batch size. claimKeys also catches any unexpected failure, rejects the call's promise and releases the keys it already holds. Added tests for 20,000 free keys and for a 20,000-key batch resumed after waiting for its first key; both fail without the change.

Claiming free keys recursed through join() -> grant() -> onGranted(), so a
batch of thousands of keys overflowed the stack, leaving the keys locked
(or an unhandled rejection with a pending promise when resumed from a
handover). Free keys are now claimed in a loop, and any unexpected failure
while claiming rejects the call and releases the keys already held.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@mykola-mokhnach
mykola-mokhnach merged commit 56e06bd into master Oct 23, 2026
5 checks passed
@mykola-mokhnach
mykola-mokhnach deleted the lock branch October 23, 2026 16:20
github-actions Bot pushed a commit that referenced this pull request Oct 23, 2026
## [6.5.0](v6.4.3...v6.5.0) (2026-10-23)

### Features

* add AsyncLock and split helpers into modules ([#90](#90)) ([56e06bd](56e06bd))
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 6.5.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

mykola-mokhnach added a commit to mykola-mokhnach/appium that referenced this pull request Oct 23, 2026
async-lock starts queued tasks from the previous task's promise chain, so a
queued task ran in the previous caller's AsyncLocalStorage store. The logger
decides masking from the store's isSensitive flag, so lines written by a
queued command (e.g. proxied request bodies) could leak sensitive values.

asyncbox's AsyncLock (appium/asyncbox#90) runs each task in the async context
of its own acquire caller. Switch all users (base-driver commands queue and
app cache, storage-plugin, plugin-test-support) to it and drop async-lock.

BREAKING CHANGE: BaseDriver#commandsQueueGuard is now an asyncbox AsyncLock.
Callback-style acquire() and the internal `queues` field are no longer
available; use promise-based acquire() and getQueueLength().

Supersedes appium#22994

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants