From 94ce27896fab54edb16c82d610b7edc8afc7a907 Mon Sep 17 00:00:00 2001 From: Julien Ripouteau Date: Thu, 14 Mar 2024 12:13:45 +0100 Subject: [PATCH] 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]>() + } }) })