From 6530e911bfc1d59da1257e599aaab01989667163 Mon Sep 17 00:00:00 2001 From: Phil Rukin Date: Sat, 25 Jun 2022 17:00:33 +0200 Subject: [PATCH] Fix release on expired locks --- src/index.ts | 36 ++++++++++++++++++++++------------ src/multi.test.ts | 26 +++++++++++++++++++++++++ src/single.test.ts | 48 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 12 deletions(-) diff --git a/src/index.ts b/src/index.ts index 850400e..07552da 100644 --- a/src/index.ts +++ b/src/index.ts @@ -313,12 +313,7 @@ export default class Redlock extends EventEmitter { settings ); - // Add 2 milliseconds to the drift to account for Redis expires precision, - // which is 1 ms, plus the configured allowable drift factor. - const drift = - Math.round( - (settings?.driftFactor ?? this.settings.driftFactor) * duration - ) + 2; + const drift = this.calculateDrift(duration, settings); return new Lock( this, @@ -351,6 +346,12 @@ export default class Redlock extends EventEmitter { lock: Lock, settings?: Partial ): Promise { + // Check if lock already expired and quit early + if (lock.expiration < Date.now()) { + const attempts: Promise[] = []; + return Promise.resolve({ attempts }); + } + // Immediately invalidate the lock. lock.expiration = 0; @@ -363,6 +364,22 @@ export default class Redlock extends EventEmitter { ); } + /** + * This method calculates drift for the provided `duration`. + */ + public calculateDrift( + duration: number, + settings?: Partial + ): number { + // Add 2 milliseconds to the drift to account for Redis expires precision, + // which is 1 ms, plus the configured allowable drift factor. + return ( + Math.round( + (settings?.driftFactor ?? this.settings.driftFactor) * duration + ) + 2 + ); + } + /** * This method extends a valid lock by the provided `duration`. */ @@ -392,12 +409,7 @@ export default class Redlock extends EventEmitter { // Invalidate the existing lock. existing.expiration = 0; - // Add 2 milliseconds to the drift to account for Redis expires precision, - // which is 1 ms, plus the configured allowable drift factor. - const drift = - Math.round( - (settings?.driftFactor ?? this.settings.driftFactor) * duration - ) + 2; + const drift = this.calculateDrift(duration, settings); const replacement = new Lock( this, diff --git a/src/multi.test.ts b/src/multi.test.ts index 5049b3e..15f9091 100644 --- a/src/multi.test.ts +++ b/src/multi.test.ts @@ -3,6 +3,10 @@ import test, { ExecutionContext } from "ava"; import Redis, { Redis as Client, Cluster } from "ioredis"; import Redlock, { ExecutionError, ResourceLockedError } from "./index.js"; +async function wait(ms: number): Promise { + return new Promise((resolve) => setTimeout(() => resolve(ms), ms)); +} + async function fail( t: ExecutionContext, error: unknown @@ -174,6 +178,28 @@ function run( } }); + test(`${namespace} - releasing expired lock doesn't fail`, async (t) => { + try { + const redlock = new Redlock([redisA, redisB, redisC]); + + const duration = 1000; + + // Acquire a lock. + const lock = await redlock.acquire(["{redlock}a"], duration); + + // Wait for duration + drift to be sure that lock has expired + await wait(duration + redlock.calculateDrift(duration)); + + // Release the lock. + await lock.release(); + t.is(await redisA.get("{redlock}a"), null, "The lock was not released"); + t.is(await redisB.get("{redlock}a"), null, "The lock was not released"); + t.is(await redisC.get("{redlock}a"), null, "The lock was not released"); + } catch (error) { + fail(t, error); + } + }); + test(`${namespace} - succeeds when a minority of clients fail`, async (t) => { try { const redlock = new Redlock([redisA, redisB, redisC]); diff --git a/src/single.test.ts b/src/single.test.ts index 6a5990d..5439370 100644 --- a/src/single.test.ts +++ b/src/single.test.ts @@ -3,6 +3,10 @@ import test, { ExecutionContext } from "ava"; import Redis, { Redis as Client, Cluster } from "ioredis"; import Redlock, { ExecutionError, ResourceLockedError } from "./index.js"; +async function wait(ms: number): Promise { + return new Promise((resolve) => setTimeout(() => resolve(ms), ms)); +} + async function fail( t: ExecutionContext, error: unknown @@ -135,6 +139,26 @@ function run(namespace: string, redis: Client | Cluster): void { } }); + test(`${namespace} - releasing expired lock doesn't fail`, async (t) => { + try { + const redlock = new Redlock([redis]); + + const duration = 1000; + + // Acquire a lock. + const lock = await redlock.acquire(["{redlock}a"], duration); + + // Wait for duration + drift to be sure that lock has expired + await wait(duration + redlock.calculateDrift(duration)); + + // Release the lock. + await lock.release(); + t.is(await redis.get("{redlock}a"), null, "The lock was not released"); + } catch (error) { + fail(t, error); + } + }); + test(`${namespace} - acquires, extends, and releases a multi-resource lock`, async (t) => { try { const redlock = new Redlock([redis]); @@ -199,6 +223,30 @@ function run(namespace: string, redis: Client | Cluster): void { } }); + test(`${namespace} - releasing expired multi-resource lock doesn't fail`, async (t) => { + try { + const redlock = new Redlock([redis]); + + const duration = 1000; + + // Acquire a lock. + const lock = await redlock.acquire( + ["{redlock}a1", "{redlock}a2"], + duration + ); + + // Wait for duration + drift to be sure that lock has expired + await wait(duration + redlock.calculateDrift(duration)); + + // Release the lock. + await lock.release(); + t.is(await redis.get("{redlock}a1"), null, "The lock was not released"); + t.is(await redis.get("{redlock}a2"), null, "The lock was not released"); + } catch (error) { + fail(t, error); + } + }); + test(`${namespace} - locks fail when redis is unreachable`, async (t) => { try { const redis = new Redis({