socket.io: Manually track client connections/disconnections

This change is required for socket.io 3.x because in 3.x
`io.sockets.clients()` no longer returns all client Socket objects.

src/node/hooks/express/socketio.js

@@ -1,31 +1,49 @@
 'use strict';
 
+const events = require('events');
 const express = require('../express');
+const log4js = require('log4js');
 const proxyaddr = require('proxy-addr');
 const settings = require('../../utils/Settings');
 const socketio = require('socket.io');
 const socketIORouter = require('../../handler/SocketIORouter');
 const hooks = require('../../../static/js/pluginfw/hooks');
 const padMessageHandler = require('../../handler/PadMessageHandler');
-const util = require('util');
 
 let io;
+const logger = log4js.getLogger('socket.io');
+const sockets = new Set();
+const socketsEvents = new events.EventEmitter();
 
 exports.expressCloseServer = async () => {
-  // According to the socket.io documentation every client is always in the default namespace (and
+  if (io == null) return;
-  // may also be in other namespaces).
+  logger.info('Closing socket.io engine...');
-  const ns = io.sockets; // The Namespace object for the default namespace.
+  // Close the socket.io engine to disconnect existing clients and reject new clients. Don't call
-  // Disconnect all socket.io clients. This probably isn't necessary; closing the socket.io Engine
+  // io.close() because that closes the underlying HTTP server, which is already done elsewhere.
-  // (see below) probably gracefully disconnects all clients. But that is not documented, and this
+  // (Closing an HTTP server twice throws an exception.) The `engine` property of socket.io Server
-  // doesn't seem to hurt, so hedge against surprising and undocumented socket.io behavior.
+  // objects is undocumented, but I don't see any other way to shut down socket.io without also
-  for (const id of await util.promisify(ns.clients.bind(ns))()) {
+  // closing the HTTP server.
-    ns.connected[id].disconnect(true);
-  }
-  // Don't call io.close() because that closes the underlying HTTP server, which is already done
-  // elsewhere. (Closing an HTTP server twice throws an exception.) The `engine` property of
-  // socket.io Server objects is undocumented, but I don't see any other way to shut down socket.io
-  // without also closing the HTTP server.
   io.engine.close();
+  // Closing the socket.io engine should disconnect all clients but it is not documented. Wait for
+  // all of the connections to close to make sure, and log the progress so that we can troubleshoot
+  // if socket.io's behavior ever changes.
+  //
+  // Note: `io.sockets.clients()` should not be used here to track the remaining clients.
+  // `io.sockets.clients()` works with socket.io 2.x, but not with 3.x: With socket.io 2.x all
+  // clients are always added to the default namespace (`io.sockets`) even if they specified a
+  // different namespace upon connection, but with socket.io 3.x clients are NOT added to the
+  // default namespace if they have specified a different namespace. With socket.io 3.x there does
+  // not appear to be a way to get all clients across all namespaces without tracking them
+  // ourselves, so that is what we do.
+  let lastLogged = 0;
+  while (sockets.size > 0) {
+    if (Date.now() - lastLogged > 1000) { // Rate limit to avoid filling logs.
+      logger.info(`Waiting for ${sockets.size} socket.io clients to disconnect...`);
+      lastLogged = Date.now();
+    }
+    await events.once(socketsEvents, 'updated');
+  }
+  logger.info('All socket.io clients have disconnected');
 };
 
 exports.expressCreateServer = (hookName, args, cb) => {
@@ -59,6 +77,15 @@ exports.expressCreateServer = (hookName, args, cb) => {
     maxHttpBufferSize: 1 << 20, // 1MiB
   });
 
+  io.on('connect', (socket) => {
+    sockets.add(socket);
+    socketsEvents.emit('updated');
+    socket.on('disconnect', () => {
+      sockets.delete(socket);
+      socketsEvents.emit('updated');
+    });
+  });
+
   io.use((socket, next) => {
     const req = socket.request;
     // Express sets req.ip but socket.io does not. Replicate Express's behavior here.