Merge pull request #1192 from shawnmcknight/redlock-opt-out

Allow opt out of redlock in redis cacher

Author: icebob

index.d.ts

@@ -815,7 +815,10 @@ declare namespace Moleculer {
 		 * @param mixinSchema Mixin schema
 		 * @param svcSchema Service schema
 		 */
-		mergeSchemas(mixinSchema: Partial<ServiceSchema>, svcSchema: Partial<ServiceSchema>): Partial<ServiceSchema>;
+		mergeSchemas(
+			mixinSchema: Partial<ServiceSchema>,
+			svcSchema: Partial<ServiceSchema>
+		): Partial<ServiceSchema>;
 
 		/**
 		 * Merge `settings` property in schema
@@ -1424,7 +1427,7 @@ declare namespace Moleculer {
 	interface RedisCacherOptions extends CacherOptions {
 		prefix?: string;
 		redis?: GenericObject;
-		redlock?: GenericObject;
+		redlock?: boolean | GenericObject;
 		monitor?: boolean;
 		pingInterval?: number;
 	}
@@ -1743,7 +1746,9 @@ declare namespace Moleculer {
 		actions: any;
 		events: any;
 
-		getServiceList<S = ServiceSettingSchema>(opts?: ServiceListCatalogOptions): ServiceSchema<S>[];
+		getServiceList<S = ServiceSettingSchema>(
+			opts?: ServiceListCatalogOptions
+		): ServiceSchema<S>[];
 		getActionList(opts?: ActionCatalogListOptions): ActionSchema[];
 	}
 

src/cachers/redis.js

@@ -6,7 +6,7 @@
 
 "use strict";
 
-let Redis, Redlock;
+let Redis;
 const BaseCacher = require("./base");
 const _ = require("lodash");
 const { METRIC } = require("../metrics");
@@ -38,6 +38,13 @@ class RedisCacher extends BaseCacher {
 		});
 
 		this.pingIntervalHandle = null;
+
+		/**
+		 * Redlock client instance
+		 * @memberof RedisCacher
+		 */
+		this.redlock = null;
+		this.redlockNonBlocking = null;
 	}
 
 	/**
@@ -94,27 +101,26 @@ class RedisCacher extends BaseCacher {
 			/* istanbul ignore next */
 			this.logger.error(err);
 		});
-		try {
-			Redlock = require("redlock");
-		} catch (err) {
-			/* istanbul ignore next */
-			this.logger.warn(
-				"The 'redlock' package is missing. If you want to enable cache lock, please install it with 'npm install redlock --save' command."
-			);
-		}
-		if (Redlock) {
-			let redlockClients = (this.opts.redlock ? this.opts.redlock.clients : null) || [
-				this.client
-			];
-			/**
-			 * redlock client instance
-			 * @memberof RedisCacher
-			 */
-			this.redlock = new Redlock(redlockClients, _.omit(this.opts.redlock, ["clients"]));
-			// Non-blocking redlock client, used for tryLock()
-			this.redlockNonBlocking = new Redlock(redlockClients, {
-				retryCount: 0
-			});
+
+		// check for !== false for backwards compatibility purposes; should be changed to opt-in in a breaking change release
+		if (this.opts.redlock !== false) {
+			let Redlock;
+			try {
+				Redlock = require("redlock");
+			} catch (err) {
+				Redlock = null;
+			}
+			if (Redlock != null) {
+				let redlockClients = (this.opts.redlock ? this.opts.redlock.clients : null) || [
+					this.client
+				];
+
+				this.redlock = new Redlock(redlockClients, _.omit(this.opts.redlock, ["clients"]));
+				// Non-blocking redlock client, used for tryLock()
+				this.redlockNonBlocking = new Redlock(redlockClients, {
+					retryCount: 0
+				});
+			}
 		}
 		if (this.opts.monitor) {
 			/* istanbul ignore next */
@@ -344,6 +350,10 @@ class RedisCacher extends BaseCacher {
 	 * @memberof RedisCacher
 	 */
 	lock(key, ttl = 15000) {
+		if (this.redlock == null) {
+			return this._handleMissingRedlock();
+		}
+
 		key = this.prefix + key + "-lock";
 		return this.redlock.lock(key, ttl).then(lock => {
 			return () => lock.unlock();
@@ -360,12 +370,27 @@ class RedisCacher extends BaseCacher {
 	 * @memberof RedisCacher
 	 */
 	tryLock(key, ttl = 15000) {
+		if (this.redlockNonBlocking == null) {
+			return this._handleMissingRedlock();
+		}
+
 		key = this.prefix + key + "-lock";
 		return this.redlockNonBlocking.lock(key, ttl).then(lock => {
 			return () => lock.unlock();
 		});
 	}
 
+	/**
+	 * Common code for handling unavailable Redlock
+	 * @returns {Promise}
+	 */
+	_handleMissingRedlock() {
+		this.logger.error(
+			"The 'redlock' package is missing or redlock is disabled. If you want to enable cache lock, please install it with 'npm install redlock --save' command."
+		);
+		return Promise.resolve();
+	}
+
 	_sequentialPromises(elements) {
 		return elements.reduce((chain, element) => {
 			return chain.then(() => this._scanDel(element));

test/unit/cachers/redis.spec.js

@@ -5,6 +5,7 @@ const C = require("../../../src/constants");
 
 jest.mock("ioredis");
 const Redis = require("ioredis");
+const BaseLogger = require("../../../src/loggers/base");
 
 Redis.mockImplementation(() => {
 	let onCallbacks = {};
@@ -646,79 +647,118 @@ describe("Test RedisCacher getWithTTL method", () => {
 	});
 });
 
-describe("Test RedisCacher lock method", () => {
-	const key = "abcd134";
-	let broker = new ServiceBroker({ logger: false });
-	let cacher = new RedisCacher({
-		ttl: 30,
-		lock: true
-	});
-	cacher.init(broker); // for empty logger
-	let unlock1, unlock2;
-	beforeEach(() => {
-		unlock1 = jest.fn(() => Promise.resolve());
-		unlock2 = jest.fn(() => Promise.resolve());
-		cacher.redlock.lock = jest.fn(() => {
-			return Promise.resolve({
-				unlock: unlock1
-			});
+describe("redlock enabled", () => {
+	describe("Test RedisCacher lock method", () => {
+		const key = "abcd134";
+		let broker = new ServiceBroker({ logger: false });
+		let cacher = new RedisCacher({
+			ttl: 30,
+			lock: true
 		});
-		cacher.redlockNonBlocking.lock = jest.fn(() => {
-			return Promise.resolve({
-				unlock: unlock2
+		cacher.init(broker); // for empty logger
+		let unlock1, unlock2;
+		beforeEach(() => {
+			unlock1 = jest.fn(() => Promise.resolve());
+			unlock2 = jest.fn(() => Promise.resolve());
+			cacher.redlock.lock = jest.fn(() => {
+				return Promise.resolve({
+					unlock: unlock1
+				});
+			});
+			cacher.redlockNonBlocking.lock = jest.fn(() => {
+				return Promise.resolve({
+					unlock: unlock2
+				});
 			});
 		});
-	});
 
-	it("should call redlock.lock when calling cacher.lock", () => {
-		return cacher.lock(key, 20).then(() => {
-			expect(cacher.redlock.lock).toHaveBeenCalledTimes(1);
-			expect(cacher.redlock.lock).toHaveBeenCalledWith(cacher.prefix + key + "-lock", 20);
+		it("should call redlock.lock when calling cacher.lock", () => {
+			return cacher.lock(key, 20).then(() => {
+				expect(cacher.redlock.lock).toHaveBeenCalledTimes(1);
+				expect(cacher.redlock.lock).toHaveBeenCalledWith(cacher.prefix + key + "-lock", 20);
+			});
 		});
-	});
 
-	it("should call redlock.unlock when calling unlock callback", () => {
-		return cacher.lock(key, 20).then(unlock => {
-			return unlock().then(() => {
-				expect(unlock1).toBeCalled();
+		it("should call redlock.unlock when calling unlock callback", () => {
+			return cacher.lock(key, 20).then(unlock => {
+				return unlock().then(() => {
+					expect(unlock1).toBeCalled();
+				});
 			});
 		});
-	});
 
-	it("should call redlock.lock when calling cacher.tryLock", () => {
-		return cacher.tryLock(key, 20).then(() => {
-			expect(cacher.redlockNonBlocking.lock).toHaveBeenCalledTimes(1);
-			expect(cacher.redlockNonBlocking.lock).toHaveBeenCalledWith(
-				cacher.prefix + key + "-lock",
-				20
-			);
+		it("should call redlock.lock when calling cacher.tryLock", () => {
+			return cacher.tryLock(key, 20).then(() => {
+				expect(cacher.redlockNonBlocking.lock).toHaveBeenCalledTimes(1);
+				expect(cacher.redlockNonBlocking.lock).toHaveBeenCalledWith(
+					cacher.prefix + key + "-lock",
+					20
+				);
+			});
 		});
-	});
 
-	it("should call redlock.unlock when calling unlock callback", () => {
-		const err = new Error("Already locked.");
-		cacher.redlockNonBlocking.lock = jest.fn(() => {
-			return Promise.reject(err);
+		it("should call redlock.unlock when calling unlock callback", () => {
+			const err = new Error("Already locked.");
+			cacher.redlockNonBlocking.lock = jest.fn(() => {
+				return Promise.reject(err);
+			});
+			return cacher.tryLock(key, 20).catch(e => {
+				expect(e).toBe(err);
+			});
 		});
-		return cacher.tryLock(key, 20).catch(e => {
-			expect(e).toBe(err);
+
+		it("should failed to acquire a lock when redlock client throw an error", () => {
+			return cacher.tryLock(key, 20);
 		});
 	});
 
-	it("should failed to acquire a lock when redlock client throw an error", () => {
-		return cacher.tryLock(key, 20);
+	describe("Test RedisCacher with opts.lock", () => {
+		it("should create redlock clients", () => {
+			let broker = new ServiceBroker({ logger: false });
+			let cacher = new RedisCacher({
+				ttl: 30
+			});
+			cacher.init(broker);
+			expect(cacher.redlock).toBeDefined();
+			expect(cacher.redlockNonBlocking).toBeDefined();
+		});
 	});
 });
 
-describe("Test RedisCacher with opts.lock", () => {
-	it("should create redlock clients", () => {
-		let broker = new ServiceBroker({ logger: false });
-		let cacher = new RedisCacher({
-			ttl: 30
-		});
-		cacher.init(broker);
-		expect(cacher.redlock).toBeDefined();
-		expect(cacher.redlockNonBlocking).toBeDefined();
+describe("redlock disabled", () => {
+	const errorMock = jest.fn();
+	class TestLogger extends BaseLogger {
+		getLogHandler() {
+			return (type, args) => {
+				if (type === "error") {
+					return errorMock(...args);
+				}
+			};
+		}
+	}
+	const logger = new TestLogger();
+
+	const broker = new ServiceBroker({ logger });
+	const cacher = new RedisCacher({ redlock: false });
+	cacher.init(broker);
+
+	afterEach(() => {
+		errorMock.mockClear();
+	});
+
+	it("should not add a redlock instance", () => {
+		expect(cacher.redlock).toBeNull();
+		expect(cacher.redlockNonBlocking).toBeNull();
+	});
+
+	it("should resolve but emit error on lock call", async () => {
+		await expect(cacher.lock()).resolves.toBeUndefined();
+		expect(errorMock).toHaveBeenCalledTimes(1);
+	});
+
+	it("should resolve but emit error on tryLock call", async () => {
+		await expect(cacher.tryLock()).resolves.toBeUndefined();
+		expect(errorMock).toHaveBeenCalledTimes(1);
 	});
 });