Enabling legacyMode and pingInterval together leads to reproducible crash

[keithmo]

Creating a redis client with legacyMode: true AND a non-zero pingInterval leads to a 100% reproducible crash.

Demonstration:

'use strict';

const redis = require('redis');

(async () => {
    const clientOptions = {
        url: 'redis://:foobar@localhost:6379/',
        legacyMode: true,
        pingInterval: 3000
    };

    const redisClient = redis.createClient(clientOptions);

    redisClient.on('error', error => {
        console.log(`redisClient: error ${error.stack}`);
    });

    await redisClient.connect();

    while (true) {
        console.log(new Date().toISOString());
        await new Promise(resolve => {
            setTimeout(resolve, 1000);
        });
    }
})();

Output:

2023-01-20T01:27:38.215Z
2023-01-20T01:27:39.216Z
2023-01-20T01:27:40.218Z
.../node_modules/@redis/client/dist/lib/client/index.js:434
            .then(reply => this.emit('ping-interval', reply))
            ^

TypeError: Cannot read properties of undefined (reading 'then')
    at Timeout._onTimeout (.../node_modules/@redis/client/dist/lib/client/index.js:434:13)
    at listOnTimeout (node:internal/timers:564:17)
    at process.processTimers (node:internal/timers:507:7)

Node.js v18.13.0

https://github.com/redis/node-redis/blob/master/packages/client/lib/client/index.ts#L365-L368 seems to assume .ping() always returns a promise.

Environment:

• Node.js Version: 18.13.0
• Redis Server Version: 7.0.7
• Node Redis Version: 4.5.1
• Platform: Mac OS 13.1 (Intel)

[benjie]

Here's a patch-package patch that seems to fix this for me (patches/@redis+client+1.4.2.patch):

diff --git a/node_modules/@redis/client/dist/lib/client/index.js b/node_modules/@redis/client/dist/lib/client/index.js
index 908d92e..7dfd2cd 100644
--- a/node_modules/@redis/client/dist/lib/client/index.js
+++ b/node_modules/@redis/client/dist/lib/client/index.js
@@ -430,7 +430,11 @@ _RedisClient_options = new WeakMap(), _RedisClient_socket = new WeakMap(), _Redi
     __classPrivateFieldSet(this, _RedisClient_pingTimer, setTimeout(() => {
         if (!__classPrivateFieldGet(this, _RedisClient_socket, "f").isReady)
             return;
-        this.ping()
+        (
+          __classPrivateFieldGet(this, _RedisClient_options, "f")?.legacyMode
+            ? this.v4.ping
+            : this.ping
+        ).call(this)
             .then(reply => this.emit('ping-interval', reply))
             .catch(err => this.emit('error', err))
             .finally(() => __classPrivateFieldGet(this, _RedisClient_instances, "m", _RedisClient_setPingTimer).call(this));

(Initially I tried (this.v4.ping ?? this.ping).call(this) but the module throws if you access .v4 without being in legacyMode.)

[keithmo]

@benjie I manually applied your patch to my local install and confirmed it fixes the issue.

Thanks for the quick response!

[benjie]

No worries, I just happened to be hit by the same issue as you just 12 hours later, by pure coincidence!

[leibale]

@keithmo @benjie thanks for sharing and even fixing the issue! :)
wanna open a PR or should I copy your fix? ;)

[benjie]

I've opened a PR but because I don't have Docker on this machine I cannot run your test suite; feel free to take over that PR.

#2386