From cc9500fe8aee5ab40efb773bd9410d2f5675412b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Cottinet?= <scottinet@protonmail.com>
Date: Mon, 26 Aug 2019 18:16:22 +0200
Subject: [PATCH] Fix a crash occuring when trying to ping dead sockets (#1407)

Description

Fix a rare crash that might occur within the WebSocket entrypoint, when a socket is closed just before a heartbeat occurs. If that happens at just the "right" time, then a heartbeat can start before that socket "close" event is processed and before that socket is marked as dead.

If that happens, pinging that socket results in an exception being thrown. Since it is not caught, this results in an unhandled exception, trapped by Kuzzle and triggering a graceful shutdown.
---
 .eslintrc.json                                |  2 +-
 .../embedded/protocols/websocket.js           | 12 ++---
 .../embedded/protocols/websocket.test.js      | 44 +++++++++++++++++++
 3 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/.eslintrc.json b/.eslintrc.json
index f69e54eb35..0f59051ced 100644
--- a/.eslintrc.json
+++ b/.eslintrc.json
@@ -47,7 +47,7 @@
     "quotes": [2, "single"],
     "semi": [2, "always"],
     "space-before-blocks": 2,
-    "space-in-parens": [2, "never"],
+    "space-in-parens": [0, "never"],
     "vars-on-top": 2,
     "yoda": [2, "never"]
   },
diff --git a/lib/api/core/entrypoints/embedded/protocols/websocket.js b/lib/api/core/entrypoints/embedded/protocols/websocket.js
index c178f2909b..414ad7f20b 100644
--- a/lib/api/core/entrypoints/embedded/protocols/websocket.js
+++ b/lib/api/core/entrypoints/embedded/protocols/websocket.js
@@ -383,8 +383,9 @@ class WebSocketProtocol extends Protocol {
 
     const connection = this.connectionPool.get(id);
 
-    if (connection && connection.alive &&
-      connection.socket.readyState === connection.socket.OPEN
+    if ( connection
+      && connection.alive
+      && connection.socket.readyState === connection.socket.OPEN
     ) {
       connection.socket.send(data);
     }
@@ -420,10 +421,9 @@ class WebSocketProtocol extends Protocol {
       lastActivityThreshold = this.activityTimestamp - this.config.heartbeat;
 
     for (const connection of this.connectionPool.values()) {
-      if (connection.alive === false) {
-        // if still marked as "not alive", then the socket did not respond
-        // to the last emitted PING request
-        // (this correctly triggers the 'close' event handler on that socket)
+      if ( connection.alive === false
+        || connection.socket.readyState !== connection.socket.OPEN
+      ) {
         connection.socket.terminate();
       } else if (connection.lastActivity < lastActivityThreshold) {
         // emit a PING request only if the socket has been inactive for longer
diff --git a/test/api/core/entrypoints/embedded/protocols/websocket.test.js b/test/api/core/entrypoints/embedded/protocols/websocket.test.js
index daef9fa4c8..0f8a405042 100644
--- a/test/api/core/entrypoints/embedded/protocols/websocket.test.js
+++ b/test/api/core/entrypoints/embedded/protocols/websocket.test.js
@@ -652,6 +652,50 @@ describe('/lib/api/core/entrypoints/embedded/protocols/websocket', () => {
       should(activeConnection.socket.terminate).not.be.called();
       should(activeConnection.socket.ping).not.be.called();
     });
+
+    it('should mark a socket as dead if not available for PING', () => {
+      const
+        Connection = function (alive, lastActivity) {
+          return {
+            alive,
+            lastActivity,
+            socket: {
+              OPEN: 'OPEN',
+              readyState: 'OPEN',
+              terminate: sinon.stub(),
+              ping: sinon.stub()
+            }
+          };
+        };
+
+      protocol.connectionPool = new Map([
+        ['ahAhAhAhStayinAliveStayinAlive', new Connection(true, 0)],
+        ['I just met you, and this is cr...gargl', new Connection(true, 0)],
+        ['ahAhAhAhStayinAliiiiiiiiive', new Connection(true, 0)]
+      ]);
+
+      const deadConnection = protocol.connectionPool.get('I just met you, and this is cr...gargl');
+      deadConnection.socket.readyState = 'CLOSED';
+
+      protocol.config.heartbeat = 1000;
+      protocol._doHeartbeat();
+
+      // inactive sockets are pinged
+      for (const id of [
+        'ahAhAhAhStayinAliveStayinAlive',
+        'ahAhAhAhStayinAliiiiiiiiive'
+      ]) {
+        const connection = protocol.connectionPool.get(id);
+
+        should(connection.alive).be.false();
+        should(connection.socket.terminate).not.be.called();
+        should(connection.socket.ping).be.calledOnce();
+      }
+
+      // dead sockets are terminated
+      should(deadConnection.socket.ping).not.be.called();
+      should(deadConnection.socket.terminate).be.calledOnce();
+    });
   });
 
   describe('#IdleTimeout', () => {