From 14a529ac044934e9f135bc30bf544f855d478fd7 Mon Sep 17 00:00:00 2001 From: Julien Ripouteau Date: Thu, 14 Mar 2024 12:11:45 +0100 Subject: [PATCH 1/3] chore: add japa/expect-type --- packages/verrou/package.json | 1 + pnpm-lock.yaml | 3 +++ 2 files changed, 4 insertions(+) diff --git a/packages/verrou/package.json b/packages/verrou/package.json index f2d8c1d..6b7a60b 100644 --- a/packages/verrou/package.json +++ b/packages/verrou/package.json @@ -45,6 +45,7 @@ }, "devDependencies": { "@aws-sdk/client-dynamodb": "^3.529.1", + "@japa/expect-type": "2.0.1", "@types/better-sqlite3": "^7.6.9", "@types/pg": "^8.11.2", "@types/proper-lockfile": "^4.1.4", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 85fc9ce..c5dfab7 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -160,6 +160,9 @@ importers: '@aws-sdk/client-dynamodb': specifier: ^3.529.1 version: 3.529.1 + '@japa/expect-type': + specifier: 2.0.1 + version: 2.0.1(@japa/runner@3.1.1) '@types/better-sqlite3': specifier: ^7.6.9 version: 7.6.9 From 94ce27896fab54edb16c82d610b7edc8afc7a907 Mon Sep 17 00:00:00 2001 From: Julien Ripouteau Date: Thu, 14 Mar 2024 12:13:45 +0100 Subject: [PATCH 2/3] refactor!: do not throw anymore when can't acquire lock --- packages/verrou/src/errors.ts | 18 +--- packages/verrou/src/lock.ts | 29 ++++-- packages/verrou/src/test_suite.ts | 21 ++-- packages/verrou/tests/lock.spec.ts | 161 ++++++++++++++++++++--------- 4 files changed, 141 insertions(+), 88 deletions(-) diff --git a/packages/verrou/src/errors.ts b/packages/verrou/src/errors.ts index fd6ce19..8d6400f 100644 --- a/packages/verrou/src/errors.ts +++ b/packages/verrou/src/errors.ts @@ -1,18 +1,10 @@ import { createError } from '@poppinss/utils' -/** - * Thrown when the lock is not acquired in the allotted time - */ -export const E_LOCK_TIMEOUT = createError( - `Lock was not acquired in the allotted time`, - 'E_LOCK_TIMEOUT', -) - /** * Thrown when user tries to update/release/extend a lock that is not acquired by them */ export const E_LOCK_NOT_OWNED = createError( - 'Looks like you are trying to update a lock that is not acquired by you', + 'Looks like you are trying to update or release a lock that is not acquired by you', 'E_LOCK_NOT_OWNED', ) @@ -23,11 +15,3 @@ export const E_LOCK_STORAGE_ERROR = createError<[{ message: string }]>( 'Lock storage error: %s', 'E_LOCK_STORAGE_ERROR', ) - -/** - * Thrown when user tries to acquire a lock that is already acquired by someone else - */ -export const E_LOCK_ALREADY_ACQUIRED = createError( - 'Lock is already acquired', - 'E_LOCK_ALREDY_ACQUIRED', -) diff --git a/packages/verrou/src/lock.ts b/packages/verrou/src/lock.ts index 69b9276..5cb75b2 100644 --- a/packages/verrou/src/lock.ts +++ b/packages/verrou/src/lock.ts @@ -2,7 +2,6 @@ import { setTimeout } from 'node:timers/promises' import { InvalidArgumentsException } from '@poppinss/utils' import { resolveDuration } from './helpers.js' -import { E_LOCK_ALREADY_ACQUIRED, E_LOCK_TIMEOUT } from './errors.js' import type { Duration, LockAcquireOptions, @@ -88,13 +87,13 @@ export class Lock { /** * Check if we reached the maximum number of attempts */ - if (attemptsDone === attemptsMax) throw new E_LOCK_TIMEOUT() + if (attemptsDone === attemptsMax) return false /** * Or check if we reached the timeout */ const elapsed = Date.now() - start - if (timeout && elapsed > timeout) throw new E_LOCK_TIMEOUT() + if (timeout && elapsed > timeout) return false /** * Otherwise wait for the delay and try again @@ -103,6 +102,7 @@ export class Lock { } this.#config.logger.debug({ key: this.#key }, 'Lock acquired') + return true } /** @@ -110,10 +110,11 @@ export class Lock { */ async acquireImmediately() { const result = await this.#lockStore.save(this.#key, this.#owner, this.#ttl) - if (!result) throw new E_LOCK_ALREADY_ACQUIRED() + if (!result) return false this.#expirationTime = this.#ttl ? Date.now() + this.#ttl : null this.#config.logger.debug({ key: this.#key }, 'Lock acquired with acquireImmediately()') + return true } /** @@ -121,10 +122,13 @@ export class Lock { * after the callback is done. * Also returns the callback return value */ - async run(callback: () => Promise): Promise { + async run(callback: () => Promise): Promise<[true, T] | [false, null]> { + const handle = await this.acquire() + if (!handle) return [false, null] + try { - await this.acquire() - return await callback() + const result = await callback() + return [true, result] } finally { await this.release() } @@ -134,12 +138,15 @@ export class Lock { * Same as `run` but try to acquire the lock immediately * Or throw an error if the lock is already acquired */ - async runImmediately(callback: () => Promise): Promise { + async runImmediately(callback: () => Promise): Promise<[true, T] | [false, null]> { + const handle = await this.acquireImmediately() + if (!handle) return [false, null] + try { - await this.acquireImmediately() - return await callback() + const result = await callback() + return [true, result] } finally { - await this.release() + if (handle) await this.release() } } diff --git a/packages/verrou/src/test_suite.ts b/packages/verrou/src/test_suite.ts index 6e0c0c6..2a56fdc 100644 --- a/packages/verrou/src/test_suite.ts +++ b/packages/verrou/src/test_suite.ts @@ -4,9 +4,9 @@ import type { Group } from '@japa/runner/core' import type { test as JapaTest } from '@japa/runner' import { setTimeout as sleep } from 'node:timers/promises' +import { E_LOCK_NOT_OWNED } from '../index.js' import { LockFactory } from './lock_factory.js' import type { LockStore } from './types/main.js' -import { E_LOCK_NOT_OWNED, E_LOCK_TIMEOUT } from '../index.js' export function registerStoreTestSuite(options: { test: typeof JapaTest @@ -73,7 +73,7 @@ export function registerStoreTestSuite(options: { // @ts-expect-error poppinss/utils typing bug }).throws(E_LOCK_NOT_OWNED.message, E_LOCK_NOT_OWNED) - test('throws timeout error when lock is not acquired in time', async () => { + test('acquire returns false when lock is not acquired in time', async ({ assert }) => { const provider = new LockFactory(options.createStore(), { retry: { timeout: 500 }, }) @@ -81,24 +81,25 @@ export function registerStoreTestSuite(options: { await lock.acquire() - await lock.acquire() - // @ts-expect-error poppinss/utils typing bug - }).throws(E_LOCK_TIMEOUT.message, E_LOCK_TIMEOUT) + const handle = await lock.acquire() + assert.isFalse(handle) + }) test('run passes result', async ({ assert }) => { const provider = new LockFactory(options.createStore()) const lock = provider.createLock('foo') - const result = await lock.run(async () => 'hello world') + const [executed, result] = await lock.run(async () => 'hello world') - assert.equal(result, 'hello world') + assert.deepEqual(executed, true) + assert.deepEqual(result, 'hello world') }) test('run passes result from a promise', async ({ assert }) => { const provider = new LockFactory(options.createStore()) const lock = provider.createLock('foo') - const result = await lock.run(async () => Promise.resolve('hello world')) + const [, result] = await lock.run(async () => Promise.resolve('hello world')) assert.equal(result, 'hello world') }) @@ -139,13 +140,13 @@ export function registerStoreTestSuite(options: { assert.isFalse(flag) - const result = await lock.run(async () => { + const [, result] = await lock.run(async () => { assert.isTrue(flag) return '42' }) assert.isTrue(flag) - assert.equal(result, '42') + assert.deepEqual(result, '42') }) test('exceptions during run do not leave mutex in locked state', async ({ assert }) => { diff --git a/packages/verrou/tests/lock.spec.ts b/packages/verrou/tests/lock.spec.ts index 748ade0..9beaa61 100644 --- a/packages/verrou/tests/lock.spec.ts +++ b/packages/verrou/tests/lock.spec.ts @@ -5,7 +5,6 @@ import { setTimeout } from 'node:timers/promises' import { Lock } from '../src/lock.js' import { MemoryStore } from '../src/drivers/memory.js' import { NullStore } from '../test_helpers/null_store.js' -import { E_LOCK_ALREADY_ACQUIRED, E_LOCK_TIMEOUT } from '../src/errors.js' const defaultOptions = { retry: { @@ -28,7 +27,7 @@ test.group('Lock', () => { assert.deepEqual(await lock.isLocked(), true) }) - test('throws timeout error when lock is not acquired in time', async ({ assert }) => { + test('return false when not acquired in time', async ({ assert }) => { class FakeStore extends NullStore { async save(_key: string) { return false @@ -40,8 +39,8 @@ test.group('Lock', () => { logger: noopLogger(), }) - // @ts-ignore - await assert.rejects(() => lock.acquire(), E_LOCK_TIMEOUT.message) + const handle = await lock.acquire() + assert.deepEqual(handle, false) }) test('respect max attempts when acquiring', async ({ assert }) => { @@ -58,8 +57,8 @@ test.group('Lock', () => { logger: noopLogger(), }) - // @ts-ignore - await assert.rejects(() => lock.acquire(), E_LOCK_TIMEOUT.message) + const handle = await lock.acquire() + assert.deepEqual(handle, false) assert.deepEqual(attempts, 5) }) @@ -76,9 +75,9 @@ test.group('Lock', () => { logger: noopLogger(), }) - // @ts-ignore - await assert.rejects(() => lock.acquire(), E_LOCK_TIMEOUT.message) + const handle = await lock.acquire() + assert.deepEqual(handle, false) const elapsed = Date.now() - start assert.isAbove(elapsed, 199) assert.isBelow(elapsed, 300) @@ -97,37 +96,13 @@ test.group('Lock', () => { logger: noopLogger(), }) - // @ts-ignore - await assert.rejects(() => lock.acquire(), E_LOCK_TIMEOUT.message) + const handle = await lock.acquire() + assert.deepEqual(handle, false) const elapsed = Date.now() - start assert.isAbove(elapsed, 100) assert.isBelow(elapsed, 200) }) - test('run should acquire and release lock', async ({ assert }) => { - assert.plan(3) - - const store = new MemoryStore() - const lock = new Lock('foo', store, defaultOptions) - - assert.deepEqual(await lock.isLocked(), false) - - await lock.run(async () => { - assert.deepEqual(await lock.isLocked(), true) - }) - - assert.deepEqual(await lock.isLocked(), false) - }) - - test('run should return callback result', async ({ assert }) => { - const store = new MemoryStore() - const lock = new Lock('foo', store, defaultOptions) - - const result = await lock.run(async () => 'foo') - - assert.deepEqual(result, 'foo') - }) - test('use default ttl when not specified', async ({ assert }) => { assert.plan(1) @@ -303,15 +278,20 @@ test.group('Lock', () => { await lock2.acquire() - // @ts-ignore - await assert.rejects(() => lock.acquire({ retry: { attempts: 1 } }), E_LOCK_TIMEOUT.message) + assert.deepEqual(await lock.acquire({ retry: { attempts: 1 } }), false) assert.deepEqual(attempts, 2) - // @ts-ignore - await assert.rejects(() => lock.acquire({ retry: { attempts: 3 } }), E_LOCK_TIMEOUT.message) + assert.deepEqual(await lock.acquire({ retry: { attempts: 3 } }), false) assert.deepEqual(attempts, 5) }) + test('acquire return true if acquired', async ({ assert }) => { + const store = new MemoryStore() + const lock = new Lock('foo', store, defaultOptions) + + assert.deepEqual(await lock.acquire(), true) + }) + test('acquire options.timeout is used', async ({ assert }) => { const store = new MemoryStore() const lock = new Lock('foo', store, { @@ -324,31 +304,43 @@ test.group('Lock', () => { }) await lock2.acquire() + const handle = lock.acquire({ + retry: { attempts: Number.POSITIVE_INFINITY, timeout: 500 }, + }) const start = Date.now() - await assert.rejects( - () => lock.acquire({ retry: { attempts: Number.POSITIVE_INFINITY, timeout: 500 } }), - // @ts-ignore - E_LOCK_TIMEOUT.message, - ) + assert.deepEqual(await handle, false) const elapsed = Date.now() - start assert.isAbove(elapsed, 500) }) + test('acquireImmediately returns true if lock is acquired', async ({ assert }) => { + const store = new MemoryStore() + const lock = new Lock('foo', store, defaultOptions) + + assert.deepEqual(await lock.isLocked(), false) + + const result = await lock.acquireImmediately() + + assert.deepEqual(result, true) + assert.deepEqual(await lock.isLocked(), true) + }) + test('acquireImmediately works', async ({ assert }) => { const store = new MemoryStore() const lock = new Lock('foo', store, defaultOptions, undefined, 1000) assert.deepEqual(await lock.isLocked(), false) - await lock.acquireImmediately() + const result = await lock.acquireImmediately() + assert.deepEqual(result, true) assert.deepEqual(await lock.isLocked(), true) assert.deepEqual(lock.getRemainingTime(), 1000) assert.deepEqual(lock.isExpired(), false) }) - test('acquireImmediately throws timeout error when lock is not available', async ({ assert }) => { + test('acquireImmediately returns false when lock is not available', async ({ assert }) => { class FakeStore extends NullStore { async save(_key: string) { return false @@ -357,8 +349,29 @@ test.group('Lock', () => { const lock = new Lock('foo', new FakeStore(), defaultOptions) - // @ts-ignore - await assert.rejects(() => lock.acquireImmediately(), E_LOCK_ALREADY_ACQUIRED.message) + assert.deepEqual(await lock.acquireImmediately(), false) + }) +}) + +test.group('Lock - run methods', () => { + test('runImmediately return false if not executed', async ({ assert }) => { + const store = new MemoryStore() + const lock = new Lock('foo', store, defaultOptions) + const lock2 = new Lock('foo', store, defaultOptions) + + await lock2.acquire() + + const result = await lock.runImmediately(async () => 'foo') + assert.deepEqual(result, [false, null]) + }) + + test('runImmediately returns true and callback result', async ({ assert }) => { + const store = new MemoryStore() + const lock = new Lock('foo', store, defaultOptions) + + const result = await lock.runImmediately(async () => 'foo') + + assert.deepEqual(result, [true, 'foo']) }) test('runImmediately should acquire and release lock', async ({ assert }) => { @@ -376,14 +389,62 @@ test.group('Lock', () => { assert.deepEqual(await lock.isLocked(), false) }) - test('runImmediately throws if lock is not available', async ({ assert }) => { + test('run should acquire and release lock', async ({ assert }) => { + assert.plan(3) + + const store = new MemoryStore() + const lock = new Lock('foo', store, defaultOptions) + + assert.deepEqual(await lock.isLocked(), false) + + await lock.run(async () => { + assert.deepEqual(await lock.isLocked(), true) + }) + + assert.deepEqual(await lock.isLocked(), false) + }) + + test('run should return callback result', async ({ assert }) => { const store = new MemoryStore() const lock = new Lock('foo', store, defaultOptions) + + const [, result] = await lock.run(async () => 'foo') + + assert.deepEqual(result, 'foo') + }) + + test('run should return false if lock is not acquired', async ({ assert }) => { + const store = new MemoryStore() + const lock = new Lock('foo', store, { retry: { attempts: 1, delay: 10 }, logger: noopLogger() }) const lock2 = new Lock('foo', store, defaultOptions) await lock2.acquire() + const result = await lock.run(async () => 'foo') + + assert.deepEqual(result, [false, null]) + }) - // @ts-expect-error - await assert.rejects(() => lock.acquireImmediately(), E_LOCK_ALREADY_ACQUIRED.message) + test('run should returns correct union type', async ({ expectTypeOf }) => { + const store = new MemoryStore() + const lock = new Lock('foo', store, defaultOptions) + + const result = await lock.run(async () => 'foo') + + expectTypeOf(result).toEqualTypeOf<[true, string] | [false, null]>() + if (result[0]) { + expectTypeOf(result).toEqualTypeOf<[true, string]>() + } + }) + + test('runImmediately should returns correct union type', async ({ expectTypeOf }) => { + const store = new MemoryStore() + const lock = new Lock('foo', store, defaultOptions) + + const result = await lock.runImmediately(async () => 'foo') + + expectTypeOf(result).toEqualTypeOf<[true, string] | [false, null]>() + if (result[0]) { + expectTypeOf(result).toEqualTypeOf<[true, string]>() + } }) }) From 1281cb53b19cb0f67d09a60aa2cf15c239a4bb4f Mon Sep 17 00:00:00 2001 From: Julien Ripouteau Date: Thu, 14 Mar 2024 12:34:56 +0100 Subject: [PATCH 3/3] doc: update doc accordingly --- docs/content/docs/api.md | 56 +++++++++++++++++-------------- docs/content/docs/introduction.md | 9 ++--- docs/content/docs/usage.md | 44 ++++++++++++------------ 3 files changed, 55 insertions(+), 54 deletions(-) diff --git a/docs/content/docs/api.md b/docs/content/docs/api.md index cf46e58..c607fb0 100644 --- a/docs/content/docs/api.md +++ b/docs/content/docs/api.md @@ -14,18 +14,38 @@ Acquire the lock. If the lock is already acquired, it will wait until it is rele ```ts const lock = verrou.createLock('key', '10s') -await lock.acquire() -await lock.acquire({ retry: { timeout: 1000 } }) -await lock.acquire({ retry: { timeout: '1s' } }) +const acquired = await lock.acquire() +const acquired = await lock.acquire({ retry: { timeout: 1000 } }) +const acquired = await lock.acquire({ retry: { timeout: '1s' } }) + +if (acquired) { + await criticalSection() +} ``` Accept an optional object with the following properties: - `retry`: An object with the following properties: - - `timeout`: The maximum time to wait for the lock to be acquired. If the timeout is reached, an `E_LOCK_TIMEOUT` error will be thrown. Defaults to `Infinity`. + - `timeout`: The maximum time to wait for the lock to be acquired. Defaults to `Infinity`. - `delay`: The delay in miliseconds between each retry. Defaults to `250` - `attempts`: The maximum number of attempts to acquire the lock. +`acquire` will return a boolean indicating if the lock was acquired or not + +### `acquireImmediately` + +Try to acquire the lock immediately ( without retrying ). If the lock is already acquired, it will return `false` immediately. + +```ts +import { errors } from '@verrou/core' + +const lock = verrou.createLock('key') +const acquired = await lock.acquireImmediately() +if (acquired) { + await criticalSection() +} +``` + ### `release` Release the lock. Note that only the lock owner can release the lock. @@ -39,45 +59,31 @@ await lock.release() ### `run` -Acquire the lock, run the callback, and release the lock. +Acquire the lock, run the callback, and release the lock. The method will return a tuple with the first value being a boolean indicating if the lock was acquired or not, and the second value being the result of the callback. ```ts const lock = verrou.createLock('key') -await lock.run(() => { +const [executed, result] = await lock.run(() => { // do something + return 'result' }) ``` + ### `runImmediately` -Same as `run`, but try to acquire the lock immediately ( without retrying ). If the lock is already acquired, it will throws a `E_LOCK_ALREADY_ACQUIRED` error. +Same as `run`, but try to acquire the lock immediately ( without retrying ). ```ts const lock = verrou.createLock('key') -await lock.runImmediately(async () => { +const [executed, result] = await lock.runImmediately(async () => { // do something + return 'result' }) ``` Accept an optional object with the same properties as `acquire`. -### `acquireImmediately` - -Try to acquire the lock immediately ( without retrying ). If the lock is already acquired, it will throws a `E_LOCK_ALREADY_ACQUIRED` error. - -```ts -import { errors } from '@verrou/core' - -const lock = verrou.createLock('key') -try { - await lock.acquireImmediately() -} catch (err) { - if (err instanceof E_LOCK_ALREADY_ACQUIRED) { - - } -} -``` - ### `isLocked` Check if the lock is acquired. diff --git a/docs/content/docs/introduction.md b/docs/content/docs/introduction.md index eda4fd8..f59e4fe 100644 --- a/docs/content/docs/introduction.md +++ b/docs/content/docs/introduction.md @@ -36,16 +36,13 @@ const verrou = new Verrou({ ```ts // title: Manual lock -import { Verrou, E_LOCK_TIMEOUT } from '@verrou/core' +import { Verrou } from '@verrou/core' const lock = verrou.createLock('my-resource') +const acquired = await lock.acquire() + try { - await lock.acquire() await doSomething() -} catch (error) { - if (error instanceof E_LOCK_TIMEOUT) { - // handle timeout - } } finally { await lock.release() } diff --git a/docs/content/docs/usage.md b/docs/content/docs/usage.md index 15edcbb..892cb87 100644 --- a/docs/content/docs/usage.md +++ b/docs/content/docs/usage.md @@ -38,13 +38,13 @@ import { verrou } from './verrou.js' const lock = verrou.createLock('my-resource', null) // We gonna wait for the lock to be acquired -await lock.acquire() - -// Do your critical code here -doSomething() +if (await lock.acquire()) { + // Do your critical code here + doSomething() -// Once you are done, release the lock. -await lock.release() + // Once you are done, release the lock. + await lock.release() +} ``` But we are still missing error handling. What if my `doSomething` method throws an error? The lock will never be released. To prevent this, always make sure to wrap your code with a try/catch/finaly block. @@ -53,10 +53,10 @@ But we are still missing error handling. What if my `doSomething` method throws import { verrou } from './verrou.js' const lock = verrou.createLock('my-resource', null) +const acquired = await lock.acquire() +if (!acquired) return 'Lock not acquired' try { - await lock.acquire() - // Do your critical code here doSomething() } catch (error) { @@ -115,29 +115,27 @@ await lock.acquire({ In general, you will either use the `retry.attempts` or `retry.timeout` options. -### Handling errors +### Handling lock acquisition failure -If ever you can't acquire a lock, an error will be thrown. You can catch it and handle it like this : +If ever you can't acquire a lock, `acquire` and `acquireImmediately` will return `false`. You can check if the lock was acquired by checking this value. ```ts import { errors } from '@verrou/core' -try { - await lock.acquire() -} catch (error) { - if (error instanceof errors.E_LOCK_TIMEOUT) { - // Handle the error - } +const acquired = await lock.acquire() +if (!acquired) { + return 'Lock not acquired' } +``` -await lock.run(async () => { - // Do your critical code here - doSomething() -}).catch(error => { - if (error instanceof errors.E_LOCK_TIMEOUT) { - // Handle the error - } +`run` and `runImmediately` methods will return a tuple with the first value being a boolean indicating if the lock was acquired or not. + +```ts +const [acquired, result] = await lock.run(async () => { + return doSomething() }) + +if (!acquired) return 'Lock not acquired' ``` ### Sharing a lock between multiple processes