fix(client): Avoids infinite promise-chaining when socket's creation fails (#2295)

* fix(client): timeout issues during tests

* fix(client): avoiding infinite Promise chaining while socket creation fails

* fix(client): Added missing semicolons

* clean test

Co-authored-by: leibale <[email protected]>

Author: JonasFaure

packages/client/lib/client/socket.spec.ts

@@ -1,5 +1,5 @@
 import { strict as assert } from 'assert';
-import { SinonFakeTimers, useFakeTimers, spy } from 'sinon';
+import { spy } from 'sinon';
 import RedisSocket, { RedisSocketOptions } from './socket';
 
 describe('Socket', () => {
@@ -9,37 +9,34 @@ describe('Socket', () => {
             options
         );
 
-        socket.on('error', (err) => {
+        socket.on('error', () => {
             // ignore errors
-            console.log(err);
         });
 
         return socket;
     }
 
     describe('reconnectStrategy', () => {
-        let clock: SinonFakeTimers;
-        beforeEach(() => clock = useFakeTimers());
-        afterEach(() => clock.restore());
-
         it('custom strategy', async () => {
+            const numberOfRetries = 10;
+
             const reconnectStrategy = spy((retries: number) => {
                 assert.equal(retries + 1, reconnectStrategy.callCount);
 
-                if (retries === 50) return new Error('50');
+                if (retries === numberOfRetries) return new Error(`${numberOfRetries}`);
 
                 const time = retries * 2;
-                queueMicrotask(() => clock.tick(time));
                 return time;
             });
 
             const socket = createSocket({
                 host: 'error',
+                connectTimeout: 1,
                 reconnectStrategy
             });
 
             await assert.rejects(socket.connect(), {
-                message: '50'
+                message: `${numberOfRetries}`
             });
 
             assert.equal(socket.isOpen, false);
@@ -48,9 +45,9 @@ describe('Socket', () => {
         it('should handle errors', async () => {
             const socket = createSocket({
                 host: 'error',
+                connectTimeout: 1,
                 reconnectStrategy(retries: number) {
                     if (retries === 1) return new Error('done');
-                    queueMicrotask(() => clock.tick(500));
                     throw new Error();
                 }
             });

packages/client/lib/client/socket.ts

@@ -105,46 +105,49 @@ export default class RedisSocket extends EventEmitter {
             throw new Error('Socket already opened');
         }
 
-        return this.#connect(0);
+        return this.#connect();
     }
 
-    async #connect(retries: number, hadError?: boolean): Promise<void> {
-        if (retries > 0 || hadError) {
-            this.emit('reconnecting');
-        }
-
-        try {
-            this.#isOpen = true;
-            this.#socket = await this.#createSocket();
-            this.#writableNeedDrain = false;
-            this.emit('connect');
+    async #connect(hadError?: boolean): Promise<void> {
+        let retries = 0;
+        do {
+            if (retries > 0 || hadError) {
+                this.emit('reconnecting');
+            }
 
             try {
-                await this.#initiator();
+                this.#isOpen = true;
+                this.#socket = await this.#createSocket();
+                this.#writableNeedDrain = false;
+                this.emit('connect');
+
+                try {
+                    await this.#initiator();
+                } catch (err) {
+                    this.#socket.destroy();
+                    this.#socket = undefined;
+                    throw err;
+                }
+                this.#isReady = true;
+                this.emit('ready');
             } catch (err) {
-                this.#socket.destroy();
-                this.#socket = undefined;
-                throw err;
-            }
-            this.#isReady = true;
-            this.emit('ready');
-        } catch (err) {
-            const retryIn = this.reconnectStrategy(retries);
-            if (retryIn instanceof Error) {
-                this.#isOpen = false;
+                const retryIn = this.reconnectStrategy(retries);
+                if (retryIn instanceof Error) {
+                    this.#isOpen = false;
+                    this.emit('error', err);
+                    throw new ReconnectStrategyError(retryIn, err);
+                }
+
                 this.emit('error', err);
-                throw new ReconnectStrategyError(retryIn, err);
+                await promiseTimeout(retryIn);
             }
-
-            this.emit('error', err);
-            await promiseTimeout(retryIn);
-            return this.#connect(retries + 1);
-        }
+            retries++;
+        } while (!this.#isReady);
     }
 
     #createSocket(): Promise<net.Socket | tls.TLSSocket> {
         return new Promise((resolve, reject) => {
-            const {connectEvent, socket} = RedisSocket.#isTlsSocket(this.#options) ?
+            const { connectEvent, socket } = RedisSocket.#isTlsSocket(this.#options) ?
                 this.#createTlsSocket() :
                 this.#createNetSocket();
 
@@ -200,7 +203,7 @@ export default class RedisSocket extends EventEmitter {
         this.#isReady = false;
         this.emit('error', err);
 
-        this.#connect(0, true).catch(() => {
+        this.#connect(true).catch(() => {
             // the error was already emitted, silently ignore it
         });
     }