Improve test converage for CnCServer and fix bugs uncovered by them

Also introduce `closeActiveWebSocket()` and `destroyActiveConnection()`
computestdev · Jan 4, 2018 · 6166ef5 · 6166ef5
1 parent c6018da
commit 6166ef5
Show file tree

Hide file tree

Showing 6 changed files with 567 additions and 34 deletions.
diff --git a/lib/CnCServer.js b/lib/CnCServer.js
@@ -4,7 +4,7 @@ const {assert} = require('chai');
 
 const URL = require('./URL');
 const log = require('./logger')({hostname: 'background', MODULE: 'CnCServer'});
-const {WS_CLOSE_NORMAL} = require('./webSocketCodes');
+const {WS_CLOSE_NORMAL, WS_CLOSE_POLICY_VIOLATION} = require('./webSocketCodes');
 const CnCServerConnection = require('./CnCServerConnection');
 
 class CnCServer {
@@ -15,8 +15,10 @@ class CnCServer {
         this.activeConnection = null;
         this.activeInstanceId = null;
         this.runScriptCount = 0;
-        this._rejectPendingRunScript = [];
+        this._rejectPendingRunScript = new Set();
         this._resolveWaitForActiveConnection = [];
+        this.lastRunScriptBegin = NaN;
+        this.lastRunScriptEnd = NaN;
         Object.seal(this);
     }
 
@@ -30,17 +32,22 @@ class CnCServer {
     }
 
     async stop() {
-        this._rejectPendingRunScript.forEach(reject => reject(Error('The server is stopping')));
-        this._rejectPendingRunScript = [];
+        this._rejectAllPendingRunScript(Error('The server is stopping'));
+        this.destroyActiveConnection(WS_CLOSE_NORMAL, 'Bye!');
+        this.activeConnection = null;
 
-        if (this.activeConnection) {
-            this.activeConnection.closeActiveWebSocket(WS_CLOSE_NORMAL, 'Bye!');
+        if (this.wsServer) {
+            this.wsServer.close();
         }
-
-        this.wsServer.close();
         this.wsServer = null;
     }
 
+    _rejectAllPendingRunScript(err) {
+        const rejects = [...this._rejectPendingRunScript];
+        this._rejectPendingRunScript.clear();
+        rejects.forEach(reject => reject(err));
+    }
+
     handleNewConnection(webSocket, httpRequest) {
         try {
             const {connection: {remoteAddress, remoteFamily, remotePort}} = httpRequest;
@@ -55,15 +62,11 @@ class CnCServer {
 
             if (oldInstanceId !== instanceId) {
                 log.warn({oldInstanceId, instanceId}, 'The browser has (re)started');
-
-                this._rejectPendingRunScript.forEach(reject => reject(Error('The browser has restarted unexpectedly')));
-                this._rejectPendingRunScript = [];
-
-                if (this.activeConnection) {
-                    this.activeConnection.closeActiveWebSocket(WS_CLOSE_NORMAL, 'Bye!');
-                    this.activeConnection = null;
-                }
-
+                this._rejectAllPendingRunScript(Error('The browser has restarted unexpectedly'));
+                this.destroyActiveConnection(
+                    WS_CLOSE_POLICY_VIOLATION,
+                    'Closing old connection because a new connection with a new instanceId has been made'
+                );
                 this.activeConnection = new CnCServerConnection();
                 this.activeInstanceId = instanceId;
             }
@@ -83,12 +86,21 @@ class CnCServer {
         assert.isOk(this.activeConnection, 'There is no active connection from the web browser');
 
         ++this.runScriptCount;
-        return await Promise.race([
-            new Promise((resolve, reject) => {
-                this._rejectPendingRunScript.push(reject);
-            }),
-            this.activeConnection.runScript({scriptContent, stackFileName}),
-        ]);
+        this.lastRunScriptBegin = Date.now();
+        let rejectPending;
+        try {
+            return await Promise.race([
+                new Promise((resolve, reject) => {
+                    rejectPending = reject;
+                    this._rejectPendingRunScript.add(rejectPending);
+                }),
+                this.activeConnection.runScript({scriptContent, stackFileName}),
+            ]);
+        }
+        finally {
+            this._rejectPendingRunScript.delete(rejectPending);
+            this.lastRunScriptEnd = Date.now();
+        }
     }
 
     async reportCodeCoverage() {
@@ -106,20 +118,38 @@ class CnCServer {
         });
     }
 
-    get lastSuccessfulPing() {
-        return this.activeConnection && this.activeConnection.lastSuccessfulPing;
+    closeActiveWebSocket(wsCode, reason) {
+        const {activeConnection} = this;
+        if (!activeConnection) {
+            return false;
+        }
+
+        return activeConnection.closeActiveWebSocket(wsCode, reason);
     }
 
-    get isRunningScript() {
-        return this.activeConnection && this.activeConnection.isRunningScript;
+    destroyActiveConnection(wsCode, reason) {
+        const {activeConnection} = this;
+        if (!activeConnection) {
+            return false;
+        }
+
+        this._rejectAllPendingRunScript(Error(reason));
+
+        this.activeConnection = null;
+        activeConnection.destroy(wsCode, reason);
+        return true;
     }
 
-    get lastRunScriptBegin() {
-        return this.activeConnection && this.activeConnection.lastRunScriptBegin;
+    get hasActiveConnection() {
+        return Boolean(this.activeConnection && this.activeConnection.hasActiveConnection);
     }
 
-    get lastRunScriptEnd() {
-        return this.activeConnection && this.activeConnection.lastRunScriptEnd;
+    get lastSuccessfulPing() {
+        return this.activeConnection ? this.activeConnection.lastSuccessfulPing : NaN;
+    }
+
+    get isRunningScript() {
+        return Boolean(this.activeConnection && this.activeConnection.isRunningScript);
     }
 
     get lastReportedVersion() {

diff --git a/lib/CnCServerConnection.js b/lib/CnCServerConnection.js
@@ -28,7 +28,7 @@ class CnCServerConnection {
         this.rpc.on('pingSuccess', delay => this._handleRpcPingSuccess(delay));
         this.activeWebSocket = null;
 
-        this.lastSuccessfulPing = null;
+        this.lastSuccessfulPing = NaN;
         this.isRunningScript = false;
         this.runScriptCount = 0;
         this.lastRunScriptBegin = NaN;
@@ -78,15 +78,29 @@ class CnCServerConnection {
     closeActiveWebSocket(code, reason) {
         const webSocket = this.activeWebSocket;
         if (!webSocket) {
-            return;
+            return false;
         }
 
         log.info({code, reason}, 'Closing active WebSocket');
         this._markWebSocketAsClosed(webSocket);
         webSocket.close(code, reason);
+        return true;
+    }
+
+    destroy(code, reason) {
+        try {
+            this.closeActiveWebSocket(code, reason);
+        }
+        finally {
+            this.rpc.end(); // abort all pending RPC calls
+        }
     }
 
     setActiveWebSocket(webSocket, httpRequest) {
+        if (this.rpc.ended) {
+            throw Error('Invalid state: this CnCServerConnection has ended');
+        }
+
         const url = new URL(`ws://localhost${httpRequest.url}`);
         const param = name => url.searchParams.get(name);
         this.lastReportedVersion = Object.freeze({

diff --git a/package.json b/package.json
@@ -74,7 +74,7 @@
     "error-stack-parser": "^2.0.1",
     "fs-extra": "^5.0.0",
     "istanbul-lib-instrument": "^1.9.1",
-    "jsonbird": "^3.0.0",
+    "jsonbird": ">= 3.1.0 < 4",
     "performr-runner-metadata-parser": "^1.0.0",
     "performr-runner-result-graph": "^5.0.0",
     "react": "^16.0.0",