diff --git a/src/node/hooks/express/socketio.js b/src/node/hooks/express/socketio.js index 178cf9155..c6ea079b4 100644 --- a/src/node/hooks/express/socketio.js +++ b/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 - // may also be in other namespaces). - const ns = io.sockets; // The Namespace object for the default namespace. - // Disconnect all socket.io clients. This probably isn't necessary; closing the socket.io Engine - // (see below) probably gracefully disconnects all clients. But that is not documented, and this - // doesn't seem to hurt, so hedge against surprising and undocumented socket.io behavior. - for (const id of await util.promisify(ns.clients.bind(ns))()) { - 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. + if (io == null) return; + logger.info('Closing socket.io engine...'); + // Close the socket.io engine to disconnect existing clients and reject new clients. 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.