Task/improve server methods (#100)

* Add newly connected clients to this.clients list - update all previous instances to use this.clients - clear this.client list when _removeClients is called - update existing tests to use this.clients - add tests to verify that the list is cleared on close * Call _removeClients on server close * Setup listeners for ws server and clear this.clients when _removeClients called * Remove all factory listener events - call clientConnected and clientDisconnected on the factory directly - remove .on('error') handler for factory since this seems unnecessary * Call 'reject' on server.listen() if server receives 'error' event * Add tests for rejecting listen() based on 'error' event * Add 'error' event listener test * Pass protocol instance to clientConnected * Overwrite clientConnected and clientDisconnected methods - http protocol instance is based on the request, not on the connected client * Use JsonRpcServerProtocol instance in clientConnected and clientDisconnected methods * Update babel and jsdoc * Add tests for clientConnected and clientDisconnected methods * Update docs for clientConnected and clientDisconnected
isaacgr · Sep 7, 2021 · a58d35e · a58d35e
1 parent b4fd2ad
commit a58d35e
Show file tree

Hide file tree

Showing 15 changed files with 1,690 additions and 1,179 deletions.
diff --git a/README.md b/README.md
@@ -396,11 +396,12 @@ _Note: The same syntax for all the above methods is used for the HTTP and WS ser
 The `clientConnected` and `clientDisconnected` methods return the host and port of the client in the event. These methods are not available for the HTTP server.
 
 ```js
-server.clientConnected = (event) => {
+server.clientConnected = (client) => {
   console.log("client connected");
 };
 
-server.clientDisconnected = (event) => {
+server.clientDisconnected = (client) => {
+  server.removeDisconnectedClient(client); // recommended to call along with clientDisconnected to clean up clients list
   console.log("client disconnected");
 };
 ```

diff --git a/package-lock.json b/package-lock.json
diff --git a/package.json b/package.json
@@ -62,7 +62,7 @@
     "prepare": "npm run test && npm run lint-fix && npm run build-docs && npm run build"
   },
   "devDependencies": {
-    "@babel/cli": "^7.4.4",
+    "@babel/cli": "^7.15.4",
     "@babel/core": "^7.4.4",
     "@babel/preset-env": "^7.4.4",
     "@babel/register": "^7.4.4",
@@ -83,7 +83,7 @@
     "eslint-plugin-standard": "^4.0.0",
     "ink-docstrap": "^1.3.2",
     "intercept-stdout": "^0.1.2",
-    "jsdoc": "^3.6.6",
+    "jsdoc": "^3.6.7",
     "jsdom": "15.1.1",
     "jsdom-global": "3.0.2",
     "mocha": "^6.1.4",

diff --git a/src/server/http.js b/src/server/http.js
@@ -41,24 +41,63 @@ class HttpServerFactory extends JsonRpcServerFactory {
   /** @inheritdoc */
   buildProtocol() {
     this.server.on("connection", (client) => {
-      this.connectedClients.push(client);
-      this.emit("clientConnected", client);
+      this.clientConnected(client);
+      this.clients.push(client);
       client.on("close", () => {
-        this.emit("clientDisconnected");
+        this.clientDisconnected(client);
       });
       // maybe need .on('end') event listener?
     });
     this.server.on("request", (request, response) => {
-      this.pcolInstance = new HttpServerProtocol(
+      const pcol = new HttpServerProtocol(
         this,
         request,
         response,
         this.options.version,
         this.options.delimiter
       );
-      this.pcolInstance.clientConnected();
+      pcol.clientConnected();
     });
   }
+
+  /**
+   * Called when client receives a `connection` event.
+   *
+   * @param {stream.Duplex} client Instance of `stream.Duplex`
+   * @returns {stream.Duplex} Returns an instance of `stream.Duplex`
+   */
+  clientConnected(client) {
+    return client;
+  }
+
+  /**
+   * Called when client disconnects from server.
+   *
+   * @param {stream.Duplex} client Instance of `stream.Duplex`
+   * @returns {object|error} Returns an object of {host, port} for the given protocol instance, or {error}
+   * if there was an error retrieving the client
+   */
+  clientDisconnected(client) {
+    return this.removeDisconnectedClient(client);
+  }
+
+  /**
+   * Removes disconnected client from `this.clients` list
+   *
+   * @param {stream.Duplex} client Instance of `stream.Duplex`
+   * @returns {object|error} Returns an object of {host, port} for the given protocol instance, or {error}
+   * if there was an error retrieving the client
+   */
+  removeDisconnectedClient(client) {
+    const clientIndex = this.clients.findIndex(c => c === client);
+    if (clientIndex === -1) {
+      return {
+        error: `Unknown client ${JSON.stringify(client)}`
+      };
+    }
+    const [disconnectedClient] = this.clients.splice(clientIndex, 1);
+    return disconnectedClient;
+  }
 }
 
 module.exports = HttpServerFactory;
diff --git a/src/server/index.js b/src/server/index.js
@@ -14,9 +14,8 @@ class JsonRpcServerFactory extends EventEmitter {
    * @param {String} [options.delimiter="\n"] Delimiter to use for [JsonRpcServerProtocol]{@link JsonRpcServerProtocol}
    * @param {Boolean} [options.exlusive=false] disallow port sharing
    * @property {object} methods Key value pairs of server method to function call
-   * @property {array} connectedClients List of connected clients
+   * @property {array} clients List of client connections which are instances of [JsonRpcServerProtocol]{@link JsonRpcServerProtocol}
    * @property {boolean} listening  Inidicates if the server is currently listening
-   * @property {class} pcolInstance Instance of [JsonRpcServerProtocol]{@link JsonRpcServerProtocol}
    */
   constructor(options) {
     super();
@@ -38,9 +37,8 @@ class JsonRpcServerFactory extends EventEmitter {
     };
 
     this.methods = {};
-    this.connectedClients = [];
+    this.clients = [];
     this.listening = false;
-    this.pcolInstance = undefined;
   }
 
   /**
@@ -50,8 +48,6 @@ class JsonRpcServerFactory extends EventEmitter {
    *
    * Establishes `error` and `close` listeners.
    *
-   * Establishes `clientConnected` and `clientDisconnected` listener.
-   *
    * @returns {Promise} Resolves host and port address for server.
    */
   listen() {
@@ -63,6 +59,7 @@ class JsonRpcServerFactory extends EventEmitter {
       const { host, port, exclusive } = this.options;
       this.setServer();
       this.server.listen({ host, port, exclusive });
+      this._setupListeners(reject);
       this.server.on("listening", () => {
         this.listening = true;
         this.buildProtocol();
@@ -71,16 +68,18 @@ class JsonRpcServerFactory extends EventEmitter {
           port: this.server.address().port
         });
       });
-      this._setupListeners();
     });
   }
 
   /**
-   * Set the `pcolInstance` for the server factory
+   * Establishes the client connection using the protocol instance
+   * and adds the newly connected client to `this.clients`.
    *
    * @abstract
    * @example
-   * this.pcolInstance = new JsonRpcClientProtocol()
+   * const pcol = JsonRpcServerProtocol()
+   * pcol.clientConnected()
+   * this.clients.push(pcol)
    */
   buildProtocol() {
     throw new Error("function must be overwritten in subclass");
@@ -98,41 +97,18 @@ class JsonRpcServerFactory extends EventEmitter {
 
   /**
    * Setup the `error` and `close` events for the factory and server.
-   * Sets `listening` to false if any errors returned or if server stops listening.
    *
-   * Calls the [JsonRpcServerFactory]{@link JsonRpcServerFactory#clientConnected} and
-   * [JsonRpcServerFactory]{@link JsonRpcServerFactory#clientDisconnected} methods
+   * Calls [JsonRpcServerFactory]{@link JsonRpcServerFactory#_removeClients} when server closes
+   * and sets `listening` to false
+   *
+   * @param {function} cb Callback to invoke if server receives an `error` event
    * @private
    */
-  _setupListeners() {
-    this.on("error", (error) => {
-      throw error;
-    });
-    this.server.on("error", (error) => {
-      throw error;
-    });
+  _setupListeners(cb) {
+    this.server.on("error", cb);
     this.server.on("close", () => {
       this.listening = false;
-    });
-    this.on("clientConnected", (client) => {
-      this.clientConnected({
-        host: client.remoteAddress,
-        port: client.remotePort
-      });
-    });
-    this.on("clientDisconnected", (client) => {
-      const clientIndex = this.connectedClients.findIndex(c => client === c);
-      if (clientIndex === -1) {
-        this.clientDisconnected({
-          error: `Unknown client ${JSON.stringify(client)}`
-        });
-      } else {
-        const [deletedClient] = this.connectedClients.splice(clientIndex, 1);
-        this.clientDisconnected({
-          host: deletedClient.remoteAddress,
-          port: deletedClient.remotePort
-        });
-      }
+      this._removeClients();
     });
   }
 
@@ -159,12 +135,15 @@ class JsonRpcServerFactory extends EventEmitter {
   /**
    * Kicks all connected clients.
    *
+   * Removes all entries from `this.clients`
+   *
    * @private
    */
   _removeClients() {
-    for (const client of this.connectedClients) {
-      client.destroy();
+    for (const pcol of this.clients) {
+      pcol.client.destroy();
     }
+    this.clients = [];
   }
 
   /**
@@ -250,12 +229,12 @@ class JsonRpcServerFactory extends EventEmitter {
       response += "]";
       response = JSON.stringify(JSON.parse(response)) + this.options.delimiter;
     }
-    if (this.connectedClients.length === 0) {
+    if (this.clients.length === 0) {
       return [Error("No clients connected")];
     }
-    return this.connectedClients.map((client) => {
+    return this.clients.map((pcol) => {
       try {
-        return this.sendNotification(client, response);
+        return this.sendNotification(pcol.client, response);
       } catch (e) {
         // possibly client disconnected
         return e;
@@ -278,6 +257,7 @@ class JsonRpcServerFactory extends EventEmitter {
    * Generate objects for notifications to send to client
    *
    * @param {Array.<string, Array>} notifications Array of notifications to send to client.
+   * @returns {JSON} Returns a valid JSON-RPC response object
    * @private
    */
   _getNotificationResponses(notifications) {
@@ -298,21 +278,45 @@ class JsonRpcServerFactory extends EventEmitter {
   }
 
   /**
-   * Called when `clientConnected` event is fired.
+   * Called when client receives a `connection` event.
+   *
+   * @param {JsonRpcServerProtocol} pcol A {@link JsonRpcServerProtocol} instance
+   * @returns {JsonRpcServerProtocol.client} Returns a client for a given `JsonRpcServerProtocol` instance
+   */
+  clientConnected(pcol) {
+    return pcol.client;
+  }
+
+  /**
+   * Called when client disconnects from server.
+   *
+   * If overwriting, its recommended to call {@link JsonRpcServerFactory.removeDisconnectedClient} manually
+   * to ensure `this.clients` is cleaned up
    *
-   * @param {object} event Returns host and port or error object
+   * @param {JsonRpcServerProtocol} pcol A {@link JsonRpcServerProtocol} instance
+   * @returns {object|error} Returns an object of {host, port} for the given protocol instance, or {error}
+   * if there was an error retrieving the client
    */
-  clientConnected(event) {
-    return event;
+  clientDisconnected(pcol) {
+    return this.removeDisconnectedClient(pcol);
   }
 
   /**
-   * Called when `clientDisconnected` event is fired.
+   * Removes disconnected client from `this.clients` list
+   *
+   * @param {JsonRpcServerProtocol} pcol A {@link JsonRpcServerProtocol} instance
+   * @returns {object|error} Returns an object of {host, port} for the given protocol instance, or {error}
    *
-   * @param {object} event Returns host and port or error object
    */
-  clientDisconnected(event) {
-    return event;
+  removeDisconnectedClient(pcol) {
+    const clientIndex = this.clients.findIndex(p => p === pcol);
+    if (clientIndex === -1) {
+      return {
+        error: `Unknown client ${JSON.stringify(pcol)}`
+      };
+    }
+    const [protocol] = this.clients.splice(clientIndex, 1);
+    return protocol.client;
   }
 
   /**

diff --git a/src/server/protocol/base.js b/src/server/protocol/base.js
@@ -31,8 +31,7 @@ class JsonRpcServerProtocol {
    * Pushes received data into `messageBuffer` and calls
    * [_waitForData]{@link JsonRpcServerProtocol#_waitForData}.
    *
-   * Registers `end` event to emit a `clientDisconnected` event
-   * on the factory
+   * Registers `end` event to call `clientDisconnected` on the factory
    *
    */
   clientConnected() {
@@ -41,7 +40,7 @@ class JsonRpcServerProtocol {
       this._waitForData();
     });
     this.client.on("end", () => {
-      this.factory.emit("clientDisconnected", this.client);
+      this.factory.clientDisconnected(this);
     });
   }
 
@@ -92,7 +91,7 @@ class JsonRpcServerProtocol {
    * Validate the request message
    *
    * @param {string} chunk
-   * @return {JSON}
+   * @returns {JSON}
    * @throws Will throw an error with a JSON-RPC error object if chunk cannot be parsed
    */
   validateRequest(chunk) {

diff --git a/src/server/tcp.js b/src/server/tcp.js
@@ -15,15 +15,15 @@ class TcpServerFactory extends JsonRpcServerFactory {
   /** @inheritdoc */
   buildProtocol() {
     this.server.on("connection", (client) => {
-      this.connectedClients.push(client);
-      this.emit("clientConnected", client);
-      this.pcolInstance = new TCPServerProtocol(
+      const pcol = new TCPServerProtocol(
         this,
         client,
         this.options.version,
         this.options.delimiter
       );
-      this.pcolInstance.clientConnected();
+      pcol.clientConnected();
+      this.clients.push(pcol);
+      this.clientConnected(pcol);
     });
   }
 }