From 5b7a87d94e3d5f25b9151ea7c76e134a04ffd72c Mon Sep 17 00:00:00 2001 From: Zihua Li Date: Mon, 28 Feb 2022 10:47:41 +0800 Subject: [PATCH] feat: skip ready check on NOPERM error Closes #1293 BREAKING CHANGE: `Redis#serverInfo` is removed. This field is never documented so you very likely have never used it. --- .eslintrc.json | 2 +- lib/Redis.ts | 8 ++++++- lib/redis/event_handler.ts | 1 - package-lock.json | 6 +++++ package.json | 1 + test/functional/connection.ts | 2 +- test/functional/monitor.ts | 4 ++-- test/functional/pipeline.ts | 2 +- test/functional/ready_check.ts | 43 +++++++++++++++++++++++++++------- 9 files changed, 53 insertions(+), 16 deletions(-) diff --git a/.eslintrc.json b/.eslintrc.json index b18ffa5d..b7930cff 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -60,7 +60,7 @@ }, "overrides": [ { - "files": ["test/**/*"], + "files": ["test/**/*", "test-cluster/**/*"], "env": { "mocha": true }, diff --git a/lib/Redis.ts b/lib/Redis.ts index 75d880fc..c737a958 100644 --- a/lib/Redis.ts +++ b/lib/Redis.ts @@ -766,6 +766,12 @@ class Redis extends Commander { const _this = this; this.info(function (err, res) { if (err) { + if (err.message && err.message.includes("NOPERM")) { + console.warn( + `Skipping the ready check because INFO command fails: "${err.message}". You can disable ready check with "enableReadyCheck". More: https://github.com/luin/ioredis/wiki/Disable-ready-check.` + ); + return callback(null, {}); + } return callback(err); } if (typeof res !== "string") { @@ -799,7 +805,7 @@ class Redis extends Commander { _this._readyCheck(callback); }, retryTime); } - }); + }).catch(noop); } } diff --git a/lib/redis/event_handler.ts b/lib/redis/event_handler.ts index 53890446..9950cb1b 100644 --- a/lib/redis/event_handler.ts +++ b/lib/redis/event_handler.ts @@ -88,7 +88,6 @@ export function connectHandler(self) { ); } } else { - self.serverInfo = info; if (self.connector.check(info)) { exports.readyHandler(self)(); } else { diff --git a/package-lock.json b/package-lock.json index 4245e6cb..41b2144b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -230,6 +230,12 @@ "integrity": "sha512-eZxlbI8GZscaGS7kkc/trHTT5xgrjH3/1n2JDwusC9iahPKWMRvRjJSAN5mCXviuTGQ/lHnhvv8Q1YTpnfz9gA==", "dev": true }, + "@types/chai": { + "version": "4.3.0", + "resolved": "https://registry.npmjs.org/@types/chai/-/chai-4.3.0.tgz", + "integrity": "sha512-/ceqdqeRraGolFTcfoXNiqjyQhZzbINDngeoAq9GoHa8PPK1yNzTaxWjA6BFWp5Ua9JpXEMSS4s5i9tS0hOJtw==", + "dev": true + }, "@types/debug": { "version": "4.1.7", "resolved": "https://registry.npmjs.org/@types/debug/-/debug-4.1.7.tgz", diff --git a/package.json b/package.json index 19dc5706..147e5df8 100644 --- a/package.json +++ b/package.json @@ -49,6 +49,7 @@ "standard-as-callback": "^2.1.0" }, "devDependencies": { + "@types/chai": "^4.3.0", "@types/debug": "^4.1.5", "@types/lodash.defaults": "^4.2.6", "@types/lodash.isarguments": "^3.1.6", diff --git a/test/functional/connection.ts b/test/functional/connection.ts index e6779442..0bf36ff2 100644 --- a/test/functional/connection.ts +++ b/test/functional/connection.ts @@ -37,7 +37,7 @@ describe("connection", function () { redis.disconnect(); setImmediate(() => done()); } - command.resolve("fake"); + return command.resolve("fake"); }); }); diff --git a/test/functional/monitor.ts b/test/functional/monitor.ts index cb9b7723..6a2cf211 100644 --- a/test/functional/monitor.ts +++ b/test/functional/monitor.ts @@ -64,10 +64,10 @@ describe("monitor", () => { it("should wait for the ready event before monitoring", (done) => { const redis = new Redis(); redis.on("ready", () => { + // @ts-expect-error const readyCheck = sinon.spy(Redis.prototype, "_readyCheck"); - redis.monitor(function (err, monitor) { + redis.monitor((err, monitor) => { expect(readyCheck.callCount).to.eql(1); - Redis.prototype._readyCheck.restore(); redis.disconnect(); monitor.disconnect(); done(); diff --git a/test/functional/pipeline.ts b/test/functional/pipeline.ts index 2ffc248e..ac73364d 100644 --- a/test/functional/pipeline.ts +++ b/test/functional/pipeline.ts @@ -144,7 +144,7 @@ describe("pipeline", () => { const redis = new Redis({ keyPrefix: "foo:" }); redis.addBuiltinCommand("someCommand"); sinon.stub(redis, "sendCommand").callsFake((command) => { - command.resolve(Buffer.from("OK")); + return command.resolve(Buffer.from("OK")); }); const result = await redis.pipeline().someCommand().exec(); expect(result).to.eql([[null, "OK"]]); diff --git a/test/functional/ready_check.ts b/test/functional/ready_check.ts index 75b458b5..c123b12d 100644 --- a/test/functional/ready_check.ts +++ b/test/functional/ready_check.ts @@ -1,19 +1,29 @@ import Redis from "../../lib/Redis"; import { noop } from "../../lib/utils"; import * as sinon from "sinon"; +import { expect } from "chai"; + +const stubInfo = ( + redis: Redis, + response: [Error | null, string | undefined] +) => { + sinon.stub(redis, "info").callsFake((section, callback) => { + const cb = typeof section === "function" ? section : callback; + const [error, info] = response; + cb(error, info); + return error ? Promise.reject(error) : Promise.resolve(info); + }); +}; describe("ready_check", () => { it("should retry when redis is not ready", (done) => { const redis = new Redis({ lazyConnect: true }); - sinon.stub(redis, "info").callsFake((callback) => { - callback(null, "loading:1\r\nloading_eta_seconds:7"); - }); + stubInfo(redis, [null, "loading:1\r\nloading_eta_seconds:7"]); + // @ts-expect-error - const stub = sinon.stub(global, "setTimeout").callsFake((body, ms) => { + sinon.stub(global, "setTimeout").callsFake((_body, ms) => { if (ms === 7000) { - redis.info.restore(); - stub.restore(); done(); } }); @@ -29,10 +39,25 @@ describe("ready_check", () => { }, }); - sinon.stub(redis, "info").callsFake((callback) => { - callback(new Error("info error")); - }); + stubInfo(redis, [new Error("info error"), undefined]); redis.connect().catch(noop); }); + + it("warns for NOPERM error", async () => { + const redis = new Redis({ + lazyConnect: true, + }); + + const warn = sinon.stub(console, "warn"); + stubInfo(redis, [ + new Error( + "NOPERM this user has no permissions to run the 'info' command" + ), + undefined, + ]); + + await redis.connect(); + expect(warn.calledOnce).to.eql(true); + }); });