Bugfix/#94/indefinite retry attempts (#95)

* Update docs to include new retry syntax. Refs #94. * Retry connection to server indefinitely if retries is set to null. Refs #94. * Clear connection timeout function when connection is ended. - with infinite retries this was not being cancelled in the back. * Add test for infinite retries. Refs #94. * Breakout connect() method into seperate functions. Refs #94. - seperate method for handling connection retry attempts - seperate method for handling socket errors * Add test for clearing timeout for connection retry. Refs #94. * Add indefinite retry logic to websocket client * Add ws client tests for indefinite retry * Test _idleTimeout value for timer since node v10.x doesnt set destroyed to true * Allow 90 char max line length.
isaacgr · Aug 24, 2021 · cbb2820 · cbb2820
1 parent 49acacf
commit cbb2820
Show file tree

Hide file tree

Showing 6 changed files with 184 additions and 79 deletions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -24,7 +24,7 @@ module.exports = {
     "consistent-return": "off",
     "max-len": [
       "error",
-      { ignoreComments: true, ignoreTemplateLiterals: true }
+      { ignoreComments: true, ignoreTemplateLiterals: true, code: 90 }
     ],
     "no-console": "off",
     "no-restricted-syntax": [

diff --git a/README.md b/README.md
@@ -1,7 +1,6 @@
 <!-- START doctoc generated TOC please keep comment here to allow auto update -->
 <!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->
 
-
 - [Jaysonic - A persistent JSON-RPC client and server](#jaysonic---a-persistent-json-rpc-client-and-server)
   - [List of features](#list-of-features)
   - [Download & Installation](#download--installation)
@@ -19,10 +18,10 @@
       - [Other client and server options](#other-client-and-server-options)
     - [Code Demos](#code-demos)
       - [Initialization](#initialization-1)
-          - [TCP](#tcp)
-          - [HTTP](#http)
-          - [HTTPS](#https)
-          - [Websocket](#websocket)
+        - [TCP](#tcp)
+        - [HTTP](#http)
+        - [HTTPS](#https)
+        - [Websocket](#websocket)
       - [Server side](#server-side)
         - [Instantiation and Listening](#instantiation-and-listening)
         - [Closing the connection](#closing-the-connection)
@@ -206,7 +205,7 @@ The client and server support changing the JSON-RPC version and the delimiter us
 
 #### Client only options
 
-`retries`: The number of retry attempts for the client to connect to the server. Default is `2`. \
+`retries`: The number of retry attempts for the client to connect to the server. Default is `2`. If set to `null` then the client will retry indefinitely.\
 `timeout`: The amount of time before a request times out. Will return a `-32000` error code. The default value is `30` (in seconds). \
 `connectionTimeout`: The amount of time between connection retry attempts to a server. The default value is `5000` (in milliseconds).
 

diff --git a/src/client/protocol/base.js b/src/client/protocol/base.js
@@ -38,6 +38,7 @@ class JsonRpcClientProtocol {
     this.pendingCalls = {};
     this.responseQueue = {};
     this.server = this.factory.server;
+    this._connectionTimeout = undefined;
     this.messageBuffer = new MessageBuffer(this.delimiter);
   }
 
@@ -60,55 +61,89 @@ class JsonRpcClientProtocol {
    * Calls [listen]{@link JsonRpcClientProtocol#listen} if connection was successful, and will resolve the promise.
    *
    * Will retry connection on the `connectionTimeout` interval.
-   * Number of connection retries is based on `remainingRetries`
+   * Number of connection retries is based on `remainingRetries`.
    *
-   * Will reject the promise if connect or re-connect attempts fail.
+   * If `null` is set for number of retries, then connections will attempt indefinitely.
    *
-   * @returns Promise
+   * Will reject the promise if connect or re-connect attempts fail and there are no remaining retries.
    *
+   * @returns Promise
    *
    */
   connect() {
-    return new Promise((resolve, reject) => {
-      const retryConnection = () => {
-        this.setConnector();
-        this.connector.connect(this.server);
-        this.connector.setEncoding("utf8");
-        this.connector.on("connect", () => {
-          this.listener = this.connector;
-          this.listen();
-          resolve(this.server);
-        });
-        this.connector.on("error", (error) => {
-          if (error.code === "ECONNREFUSED" && this.factory.remainingRetries) {
-            this.factory.remainingRetries -= 1;
-            console.error(
-              `Unable to connect. Retrying. ${this.factory.remainingRetries} attempts left.`
-            );
-            setTimeout(() => {
-              retryConnection();
-            }, this.factory.connectionTimeout);
-          } else {
-            this.factory.pcolInstance = undefined;
-            reject(error);
-          }
-        });
-        this.connector.on("close", () => {
-          this.factory.emit("serverDisconnected");
-        });
-      };
-      return retryConnection();
+    return new Promise((resolve, reject) => this._retryConnection(resolve, reject));
+  }
+
+  /**
+   *
+   * Manage the connection attempts for the client.
+   *
+   * @param {Promise.resolve} resolve `Promise.resolve` passed from [connect]{@link JsonRpcClientProtocol#connect}
+   * @param {Promise.reject} reject `Promise.reject` passed from [connect]{@link JsonRpcClientProtocol#connect}
+   *
+   * @returns Promise
+   *
+   * @private
+   */
+  _retryConnection(resolve, reject) {
+    this.setConnector();
+    this.connector.connect(this.server);
+    this.connector.setEncoding("utf8");
+    this.connector.on("connect", () => {
+      this.listener = this.connector;
+      this.listen();
+      resolve(this.server);
     });
+    this.connector.on("error", error => this._onConnectionFailed(error, resolve, reject));
+    this.connector.on("close", () => {
+      this.factory.emit("serverDisconnected");
+    });
+  }
+
+  /**
+   *
+   * Handle connection attempt errors. Log failures and
+   * retry if required.
+   *
+   *
+   * @param {Error} error `node.net` system error (https://nodejs.org/api/errors.html#errors_common_system_errors)
+   * @param {Promise.resolve} resolve `Promise.resolve` passed from [connect]{@link JsonRpcClientProtocol#connect}
+   * @param {Promise.reject} reject `Promise.reject` passed from [connect]{@link JsonRpcClientProtocol#connect}
+   *
+   * @returns Promise
+   *
+   * @private
+   */
+  _onConnectionFailed(error, resolve, reject) {
+    if (this.factory.remainingRetries > 0) {
+      this.factory.remainingRetries -= 1;
+      console.error(
+        `Failed to connect. Address [${this.server.host}:${this.server.port}]. Retrying. ${this.factory.remainingRetries} attempts left.`
+      );
+    } else if (this.factory.remainingRetries === 0) {
+      this.factory.pcolInstance = undefined;
+      return reject(error);
+    } else {
+      console.error(
+        `Failed to connect. Address [${this.server.host}:${this.server.port}]. Retrying.`
+      );
+    }
+    this._connectionTimeout = setTimeout(() => {
+      this._retryConnection(resolve, reject);
+    }, this.factory.connectionTimeout);
   }
 
   /**
    * Ends connection to the server.
    *
    * Sets `JsonRpcClientFactory.pcolInstance` to `undefined`
    *
+   * Clears the connection timeout
+   *
    * @param {function} cb Called when connection is sucessfully closed
    */
   end(cb) {
+    clearTimeout(this._connectionTimeout);
     this.factory.pcolInstance = undefined;
     this.connector.end(cb);
   }

diff --git a/src/client/protocol/ws.js b/src/client/protocol/ws.js
@@ -30,49 +30,54 @@ class WsClientProtocol extends JsonRpcClientProtocol {
   /**
    * @inheritdoc
    */
-  connect() {
-    return new Promise((resolve, reject) => {
-      const retryConnection = () => {
-        this.setConnector();
-        this.connector.onopen = (event) => {
-          this.listener = this.connector;
-          this.listen();
-          resolve(event);
-        };
-        this.connector.onerror = (error) => {
-          // let the onclose event got it otherwise
-          if (error.error && error.error.code !== "ECONNREFUSED") {
-            reject(error);
-          }
-        };
-        this.connector.onclose = (event) => {
-          if (this.connector.__clientClosed) {
-            // we dont want to retry if the client purposefully closed the connection
-            console.log(
-              `Client closed connection. Code [${event.code}]. Reason [${event.reason}].`
-            );
-          } else {
-            if (this.factory.remainingRetries === 0) {
-              this.factory.pcolInstance = undefined;
-              reject(event);
-            } else {
-              this.factory.remainingRetries -= 1;
-              console.error(
-                `Connection failed. ${this.factory.remainingRetries} attempts left.`
-              );
-            }
-            setTimeout(() => {
-              retryConnection();
-            }, this.factory.connectionTimeout);
-          }
-        };
-      };
-      return retryConnection();
-    });
+  _retryConnection(resolve, reject) {
+    this.setConnector();
+    this.connector.onopen = (event) => {
+      this.listener = this.connector;
+      this.listen();
+      resolve(event);
+    };
+    this.connector.onerror = (error) => {
+      // let the onclose event get it otherwise
+      if (error.error && error.error.code !== "ECONNREFUSED") {
+        reject(error);
+      }
+    };
+    this.connector.onclose = (event) => {
+      if (this.connector.__clientClosed) {
+        // we dont want to retry if the client purposefully closed the connection
+        console.log(
+          `Client closed connection. Code [${event.code}]. Reason [${event.reason}].`
+        );
+      } else {
+        return this._onConnectionFailed(event, resolve, reject);
+      }
+    };
+  }
+
+  /**
+   * @inheritdoc
+   */
+  _onConnectionFailed(event, resolve, reject) {
+    if (this.factory.remainingRetries > 0) {
+      this.factory.remainingRetries -= 1;
+      console.error(
+        `Failed to connect. Address [${this.url}]. Retrying. ${this.factory.remainingRetries} attempts left.`
+      );
+    } else if (this.factory.remainingRetries === 0) {
+      this.factory.pcolInstance = undefined;
+      return reject(event);
+    } else {
+      console.error(`Failed to connect. Address [${this.url}]. Retrying.`);
+    }
+    this._connectionTimeout = setTimeout(() => {
+      this._retryConnection(resolve, reject);
+    }, this.factory.connectionTimeout);
   }
 
   /** @inheritdoc */
   end(code, reason) {
+    clearTimeout(this._connectionTimeout);
     this.factory.pcolInstance = undefined;
     this.connector.__clientClosed = true; // used to determine if client initiated close event
     this.connector.close(code, reason);

diff --git a/tests/connections/base-client-protocol.test.js b/tests/connections/base-client-protocol.test.js
@@ -3,6 +3,7 @@ const intercept = require("intercept-stdout");
 const Jaysonic = require("../../src");
 
 const client = new Jaysonic.client.tcp({ retries: 1 });
+const infClient = new Jaysonic.client.tcp({ retries: null });
 
 describe("Base Client Reconnect", () => {
   it("should retry the connection to the server and log the attempts", (done) => {
@@ -14,11 +15,43 @@ describe("Base Client Reconnect", () => {
     setTimeout(() => {
       unhook();
       expect(capturedText).to.equal(
-        "Unable to connect. Retrying. 0 attempts left.\n"
+        "Failed to connect. Address [127.0.0.1:8100]. Retrying. 0 attempts left.\n"
       );
     }, 100);
     conn.catch(() => {
       done();
     });
   }).timeout(10000);
+  it("should retry the connection to the server indefinitely if retries set to 'null'", (done) => {
+    infClient.connect();
+    let capturedText = "";
+    const unhook = intercept((text) => {
+      capturedText += text;
+    });
+    setTimeout(() => {
+      unhook();
+      expect(capturedText).to.equal(
+        "Failed to connect. Address [127.0.0.1:8100]. Retrying.\n"
+      );
+      infClient.end();
+      done();
+    }, 100);
+  }).timeout(10000);
+  it("should clear the _connectionTimeout of the protocol instance when connection ended", (done) => {
+    infClient.connect();
+    let capturedText = "";
+    const unhook = intercept((text) => {
+      capturedText += text;
+    });
+    setTimeout(() => {
+      unhook();
+      expect(capturedText).to.equal(
+        "Failed to connect. Address [127.0.0.1:8100]. Retrying.\n"
+      );
+      const pcolConnectionTimeout = infClient.pcolInstance._connectionTimeout;
+      infClient.end();
+      expect(pcolConnectionTimeout._idleTimeout).to.equal(-1); // test _idleTimeout since _destroyed is not set immediately in v10.x
+      done();
+    }, 1000);
+  }).timeout(10000);
 });
diff --git a/tests/connections/ws-client-protocol.test.js b/tests/connections/ws-client-protocol.test.js
@@ -4,6 +4,7 @@ const Jaysonic = require("../../src");
 const { wss } = require("../test-server");
 
 const wsClient = new Jaysonic.client.ws({ retries: 1 });
+const infClient = new Jaysonic.client.ws({ retries: null });
 
 describe("WS Client Reconnect", () => {
   it("should attempt to reconnect to the server and reject promise if unable", (done) => {
@@ -13,6 +14,38 @@ describe("WS Client Reconnect", () => {
       done();
     });
   }).timeout(10000);
+  it("should retry the connection to the server indefinitely if retries set to 'null'", (done) => {
+    infClient.connect();
+    let capturedText = "";
+    const unhook = intercept((text) => {
+      capturedText += text;
+    });
+    setTimeout(() => {
+      unhook();
+      expect(capturedText).to.equal(
+        "Failed to connect. Address [ws://127.0.0.1:8100]. Retrying.\n"
+      );
+      infClient.end();
+      done();
+    }, 100);
+  }).timeout(10000);
+  it("should clear the _connectionTimeout of the protocol instance when connection ended", (done) => {
+    infClient.connect();
+    let capturedText = "";
+    const unhook = intercept((text) => {
+      capturedText += text;
+    });
+    setTimeout(() => {
+      unhook();
+      expect(capturedText).to.equal(
+        "Failed to connect. Address [ws://127.0.0.1:8100]. Retrying.\n"
+      );
+      const pcolConnectionTimeout = infClient.pcolInstance._connectionTimeout;
+      infClient.end();
+      expect(pcolConnectionTimeout._idleTimeout).to.equal(-1); // test _idleTimeout since _destroyed is not set immediately in v10.x
+      done();
+    }, 1000);
+  }).timeout(10000);
 });
 
 describe("WS Client end connection", () => {