From 714e5904961dbf91c340a240265ed05706ee91ce Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Fri, 24 Sep 2021 18:16:17 +0200 Subject: [PATCH 01/14] Add sorting to web interface tables Add sort options to the slave, instance, user, role and plugins tables. --- .../web_ui/src/components/InstancesPage.jsx | 22 +++++++++++++------ .../web_ui/src/components/PluginsPage.jsx | 6 +++++ packages/web_ui/src/components/RolesPage.jsx | 3 +++ packages/web_ui/src/components/SlavesPage.jsx | 8 +++++++ packages/web_ui/src/components/UsersPage.jsx | 7 ++++++ 5 files changed, 39 insertions(+), 7 deletions(-) diff --git a/packages/web_ui/src/components/InstancesPage.jsx b/packages/web_ui/src/components/InstancesPage.jsx index e7020a05..85996fe2 100644 --- a/packages/web_ui/src/components/InstancesPage.jsx +++ b/packages/web_ui/src/components/InstancesPage.jsx @@ -11,6 +11,8 @@ import { useInstanceList } from "../model/instance"; import { notifyErrorHandler } from "../util/notify"; import { useSlaveList } from "../model/slave"; +const strcmp = new Intl.Collator(undefined, { numerice: "true", sensitivity: "base" }).compare; + function CreateInstanceButton(props) { let control = useContext(ControlContext); @@ -64,6 +66,14 @@ export default function InstancesPage() { let [slaveList] = useSlaveList(); let [instanceList] = useInstanceList(); + function slaveName(slaveId) { + let slave = slaveList.find(s => s.id === slaveId); + if (slave) { + return slave.name; + } + return String(slaveId); + } + return strcmp(a["name"], b["name"]), }, { title: "Assigned Slave", key: "assigned_slave", - render: instance => { - let slave = slaveList.find(s => s.id === instance.assigned_slave); - if (slave) { - return slave.name; - } - return instance.assigned_slave; - }, + render: instance => slaveName(instance["assigned_slave"]), + sorter: (a, b) => strcmp(slaveName(a["assigned_slave"]), slaveName(b["assigned_slave"])), }, { title: "Status", dataIndex: "status", + sorter: (a, b) => strcmp(a["status"], b["status"]), }, ]} dataSource={instanceList} diff --git a/packages/web_ui/src/components/PluginsPage.jsx b/packages/web_ui/src/components/PluginsPage.jsx index e93483bc..3c3c5a46 100644 --- a/packages/web_ui/src/components/PluginsPage.jsx +++ b/packages/web_ui/src/components/PluginsPage.jsx @@ -6,6 +6,8 @@ import notify from "../util/notify"; import ControlContext from "./ControlContext"; import PageLayout from "./PageLayout"; +const strcmp = new Intl.Collator(undefined, { numerice: "true", sensitivity: "base" }).compare; + export default function PluginsPage() { let plugins = useContext(ControlContext).plugins; @@ -47,6 +49,8 @@ export default function PluginsPage() { title: "Name", key: "name", render: plugin => (plugin.info ? plugin.info.title : plugin.meta.name), + defaultSortOrder: "ascend", + sorter: (a, b) => strcmp(a.info ? a.info.title : a.meta.name, b.info ? b.info.title : b.meta.name), }, { title: "Version", @@ -60,11 +64,13 @@ export default function PluginsPage() { } return plugin.package.version; }, + sorter: (a, b) => strcmp(a.meta.version, b.meta.version), }, { title: "Loaded", dataIndex: ["meta", "loaded"], render: loaded => (loaded ? "Yes" : null), + sorter: (a, b) => a.loaded - b.loaded, responsive: ["sm"], }, ]} diff --git a/packages/web_ui/src/components/RolesPage.jsx b/packages/web_ui/src/components/RolesPage.jsx index 388a48f1..fc1ba59a 100644 --- a/packages/web_ui/src/components/RolesPage.jsx +++ b/packages/web_ui/src/components/RolesPage.jsx @@ -9,6 +9,8 @@ import ControlContext from "./ControlContext"; import { notifyErrorHandler } from "../util/notify"; import PageLayout from "./PageLayout"; +const strcmp = new Intl.Collator(undefined, { numerice: "true", sensitivity: "base" }).compare; + function CreateRoleButton() { let control = useContext(ControlContext); @@ -81,6 +83,7 @@ export default function RolesPage() { { title: "Name", dataIndex: "name", + sorter: (a, b) => strcmp(a["name"], b["name"]), }, { title: "Description", diff --git a/packages/web_ui/src/components/SlavesPage.jsx b/packages/web_ui/src/components/SlavesPage.jsx index 15a83f3b..f94001a8 100644 --- a/packages/web_ui/src/components/SlavesPage.jsx +++ b/packages/web_ui/src/components/SlavesPage.jsx @@ -9,6 +9,8 @@ import PageLayout from "./PageLayout"; import { useSlaveList } from "../model/slave"; import { notifyErrorHandler } from "../util/notify"; +const strcmp = new Intl.Collator(undefined, { numerice: "true", sensitivity: "base" }).compare; + function GenerateSlaveTokenButton(props) { let control = useContext(ControlContext); @@ -86,19 +88,25 @@ export default function SlavesPage() { { title: "Name", dataIndex: "name", + defaultSortOrder: "ascend", + sorter: (a, b) => strcmp(a["name"], b["name"]), }, { title: "Agent", dataIndex: "agent", + sorter: (a, b) => strcmp(a["agent"], b["agent"]), + responsive: ["lg"], }, { title: "Version", dataIndex: "version", + sorter: (a, b) => strcmp(a["version"], b["version"]), }, { title: "Connected", key: "connected", render: slave => slave["connected"] && "Yes", + sorter: (a, b) => a["connected"] - b["connected"], }, ]} dataSource={slaveList} diff --git a/packages/web_ui/src/components/UsersPage.jsx b/packages/web_ui/src/components/UsersPage.jsx index c8f98376..296fd1ce 100644 --- a/packages/web_ui/src/components/UsersPage.jsx +++ b/packages/web_ui/src/components/UsersPage.jsx @@ -9,6 +9,8 @@ import ControlContext from "./ControlContext"; import { notifyErrorHandler } from "../util/notify"; import PageLayout from "./PageLayout"; +const strcmp = new Intl.Collator(undefined, { numerice: "true", sensitivity: "base" }).compare; + function CreateUserButton() { let control = useContext(ControlContext); @@ -80,6 +82,8 @@ export default function UsersPage() { { title: "Name", dataIndex: "name", + defaultSortOrder: "ascend", + sorter: (a, b) => strcmp(a["name"], b["name"]), }, { title: "Roles", @@ -90,18 +94,21 @@ export default function UsersPage() { title: "Admin", key: "admin", render: user => user["is_admin"] && "yes", + sorter: (a, b) => a["is_admin"] - b["is_admin"], responsive: ["lg"], }, { title: "Whitelisted", key: "whitelisted", render: user => user["is_whitelisted"] && "yes", + sorter: (a, b) => a["is_whitelisted"] - b["is_whitelisted"], responsive: ["lg"], }, { title: "Banned", key: "banned", render: user => user["is_banned"] && "yes", + sorter: (a, b) => a["is_banned"] - b["is_banned"], responsive: ["lg"], }, ]} From d99e160a272af3e3dfdb2c8511e165b98ab25c4b Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Mon, 27 Sep 2021 03:20:27 +0200 Subject: [PATCH 02/14] Remove unused node-factorio-api dependency --- packages/master/package.json | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/master/package.json b/packages/master/package.json index 76a2fb6d..435983a7 100644 --- a/packages/master/package.json +++ b/packages/master/package.json @@ -26,7 +26,6 @@ "fs-extra": "^8.1.0", "jsonwebtoken": "^8.2.1", "jszip": "^3.2.2", - "node-factorio-api": "^0.3.8", "set-blocking": "^2.0.0", "winston": "^3.3.3", "ws": "^7.2.3", From 642504874bdef076dd5dd8b3a7419d44efa8cf19 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Mon, 27 Sep 2021 03:21:19 +0200 Subject: [PATCH 03/14] Fix error on slave reconnect of stale connection Remove call to removed function setTimeout on connector that was left over from the connector refactor. --- packages/master/src/WsServer.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/master/src/WsServer.js b/packages/master/src/WsServer.js index 9c383976..ea08fd91 100644 --- a/packages/master/src/WsServer.js +++ b/packages/master/src/WsServer.js @@ -266,7 +266,6 @@ ${err.stack}` let connection = this.slaveConnections.get(data.id); if (connection) { logger.verbose(`WsServer | disconnecting existing connection for slave ${data.id}`); - connection.connector.setTimeout(15); // Slave connection is likely stalled await connection.disconnect(1008, "Registered from another connection"); } From f529457f96c905135b2ff82ee770b9ab215d578a Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Mon, 27 Sep 2021 03:39:06 +0200 Subject: [PATCH 04/14] Replace ini packages with homebrew module The INI format Factorio use differs from what the ini package parse and produce. Most notably no escape mechanism exists in Factorio's parser which causes instance paths containing among other thing the # character to completely break. Fix by replacing the ini package with a Factorio compatible implementation of INI parsing and serialization. Resolves the problem in #414. --- packages/lib/factorio/export.js | 6 +-- packages/lib/factorio/server.js | 4 +- packages/lib/ini.js | 94 +++++++++++++++++++++++++++++++++ packages/lib/package.json | 1 - test/lib/ini.js | 93 ++++++++++++++++++++++++++++++++ 5 files changed, 192 insertions(+), 6 deletions(-) create mode 100644 packages/lib/ini.js create mode 100644 test/lib/ini.js diff --git a/packages/lib/factorio/export.js b/packages/lib/factorio/export.js index 2e71dd08..173f5cb3 100644 --- a/packages/lib/factorio/export.js +++ b/packages/lib/factorio/export.js @@ -2,10 +2,10 @@ "use strict"; const fs = require("fs-extra"); const path = require("path"); -const ini = require("ini"); const Jimp = require("jimp"); const JSZip = require("jszip"); +const libIni = require("../ini"); const libBuildMod = require("../build_mod"); /** @@ -312,7 +312,7 @@ async function exportLocale(server, modVersions, modOrder, languageCode) { } let baseLocaleFilePath = server.dataPath("base", "locale", languageCode, "base.cfg"); - mergeLocale(ini.parse(await fs.readFile(baseLocaleFilePath, "utf8"))); + mergeLocale(libIni.parse(await fs.readFile(baseLocaleFilePath, "utf8"))); for (let mod of modOrder) { if (["core", "base", "export"].includes(mod)) { @@ -330,7 +330,7 @@ async function exportLocale(server, modVersions, modOrder, languageCode) { } for (let file of zip.file(new RegExp(`locale\\/${languageCode}\\/.*\\.cfg`))) { let content = await file.async("nodebuffer"); - mergeLocale(ini.parse(content.toString("utf8"))); + mergeLocale(libIni.parse(content.toString("utf8"))); } } diff --git a/packages/lib/factorio/server.js b/packages/lib/factorio/server.js index 5b08447a..ec2d4766 100644 --- a/packages/lib/factorio/server.js +++ b/packages/lib/factorio/server.js @@ -7,7 +7,7 @@ const events = require("events"); const util = require("util"); const crypto = require("crypto"); -const ini = require("ini"); +const libIni = require("../ini"); const rconClient = require("rcon-client"); const libErrors = require("../errors"); const { logger } = require("../logging"); @@ -1167,7 +1167,7 @@ class FactorioServer extends events.EventEmitter { } async _writeConfigIni() { - let content = ini.encode({ + let content = libIni.stringify({ path: { "read-data": this.dataPath(), "write-data": this.writePath(), diff --git a/packages/lib/ini.js b/packages/lib/ini.js new file mode 100644 index 00000000..413aeb92 --- /dev/null +++ b/packages/lib/ini.js @@ -0,0 +1,94 @@ +/** + * Factorio Compatible INI parser + * + * @module lib/ini + * @author Hornwitser + */ + +"use strict"; + +/** + * Parse INI file into object of sections + * @param {string} input - content of INI file to parse. + * @returns {object>} + * sections parsed from the ini file. + */ +function parse(input) { + const sections = {}; + let currentSection = sections; + const lines = input.split(/\r\n|\n/); + for (let number = 1; number <= lines.length; number++) { + let line = lines[number - 1].replace(/^[\t ]+/, ""); + if (!line || line[0] === "#" || line[0] === ";") { + continue; + } + + // Read section header + if (line[0] === "[") { + line = line.replace(/[\t ]+$/, ""); + if (line.slice(-1) !== "]") { + throw new Error(`Unterminated section header on line ${number}`); + } + const name = line.slice(1, -1); + if (Object.prototype.hasOwnProperty.call(sections, name)) { + throw new Error(`Duplicated section [${name}] on line ${number}`); + } + currentSection = {}; + sections[name] = currentSection; + continue; + } + + // Read key value + const equalIndex = line.indexOf("="); + if (equalIndex === -1) { + throw new Error(`Missing value for key ${line} on line ${number}`); + } + const key = line.slice(0, equalIndex); + if (Object.prototype.hasOwnProperty.call(currentSection, key)) { + throw new Error(`Duplicated key ${key} on line ${number}`); + } + const value = line.slice(equalIndex + 1); + currentSection[key] = value; + } + + return sections; +} + +/** + * Serialize an object of section definitions into an INI file + * @param {object>} sections - + * Sections to encode into ini format + * @returns {string} sections serialized into INI format + */ +function stringify(sections) { + let serialized = []; + let first = true; + + for (let [key, value] of Object.entries(sections)) { + if (typeof value === "string") { + serialized.push(`${key}=${value}`); + first = false; + } + } + + for (let [section, content] of Object.entries(sections)) { + if (typeof content === "object") { + if (!first) { + serialized.push(""); + } + first = false; + serialized.push(`[${section}]`); + for (let [key, value] of Object.entries(content)) { + serialized.push(`${key}=${value}`); + } + } + } + + if (serialized.length) { serialized.push(""); } + return serialized.join("\n"); +} + +module.exports = { + parse, + stringify, +}; diff --git a/packages/lib/package.json b/packages/lib/package.json index 63e9d6a3..f7b78901 100644 --- a/packages/lib/package.json +++ b/packages/lib/package.json @@ -15,7 +15,6 @@ "ajv": "^6.12.6", "chalk": "^4.1.0", "fs-extra": "^8.1.0", - "ini": "^1.3.5", "isomorphic-ws": "^4.0.1", "jimp": "^0.12.1", "jsonwebtoken": "^8.2.1", diff --git a/test/lib/ini.js b/test/lib/ini.js new file mode 100644 index 00000000..846e614c --- /dev/null +++ b/test/lib/ini.js @@ -0,0 +1,93 @@ +"use strict"; +const assert = require("assert").strict; +const libIni = require("@clusterio/lib/ini"); + +describe("lib/ini", function() { + describe("parse()", function() { + it("should ignore comments", function() { + assert.deepEqual( + libIni.parse("# A comment\n; Another comment\nfoo=bar\n"), + { foo: "bar" }, + ); + }); + it("should ignore certain whitespace", function() { + assert.deepEqual( + libIni.parse(" # A comment \n\n [spam] \t \n \tfoo=bar\n"), + { spam: { foo: "bar" }} + ); + }); + it("should preserve certain whitespace", function() { + assert.deepEqual( + libIni.parse("[\t spam\t ]\nfoo =\t bar \n"), + { "\t spam\t ": { "foo ": "\t bar " }} + ); + }); + it("should not treat characters specially", function() { + assert.deepEqual( + libIni.parse("[#;spam]\nfoo#;=#;bar=#;'\""), + { "#;spam": { "foo#;": "#;bar=#;'\"" }} + ); + }); + it("should handle windows line endings", function() { + assert.deepEqual( + libIni.parse("[spam]\r\nfoo=bar\r\n"), + { "spam": { "foo": "bar" }} + ); + }); + it("should throw on incomplete section", function() { + assert.throws( + () => libIni.parse("[spam"), + new Error("Unterminated section header on line 1") + ); + }); + it("should throw on missing value", function() { + assert.throws( + () => libIni.parse("key"), + new Error("Missing value for key key on line 1") + ); + }); + it("should throw on duplicated section", function() { + assert.throws( + () => libIni.parse("[spam]\n[spam]\n"), + new Error("Duplicated section [spam] on line 2") + ); + }); + it("should throw on duplicated keys", function() { + assert.throws( + () => libIni.parse("key=value\nkey=value\n"), + new Error("Duplicated key key on line 2") + ); + }); + }); + describe("stringify()", function() { + it("should serialize to reference output", function() { + let cases = [ + [{}, ""], + [{ foo: "bar" }, "foo=bar\n"], + [{ foo: { spam: "one", bar: "two" }}, "[foo]\nspam=one\nbar=two\n"], + [ + { start: "alpha", foo: { spam: "one", bar: "two" }, end: "beta" }, + "start=alpha\nend=beta\n\n[foo]\nspam=one\nbar=two\n", + ], + [{ "trail ": "\t tabs \t" }, "trail =\t tabs \t\n"], + [{ "#;spam": { "foo#;": "#;bar=#;'\"" }}, "[#;spam]\nfoo#;=#;bar=#;'\"\n"], + ]; + for (let [input, expected] of cases) { + assert.equal(libIni.stringify(input), expected); + } + }); + }); + it("should round trip object", function() { + for (let expected of [ + {}, + { simple: "value" }, + { section: { foo: "false", bar: "text" }}, + { mixed: "root", section: { foo: "1", bar: "2" }, end: "value" }, + { first: { foo: "1", bar: "2" }, second: { spam: "true" }}, + { '"#;[] ': ' "#;[] ', ' #;"[ ': { '"#;[] ': ' "#;[ ' }}, + ]) { + let actual = libIni.parse(libIni.stringify(expected)); + assert.deepEqual(actual, expected); + } + }); +}); From 4a72c14b743914fc95cce135cd712f3276c14278 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Tue, 28 Sep 2021 21:21:30 +0200 Subject: [PATCH 05/14] Terminate connection on timeout If a connection has timed out the most likely cause is the connection going stale, in which case calling close could take up to 30 seconds before the ws lib gives up on waiting for the close message. Terminate the connection instead on the Node.js side. Note that Firefox does not permit applications from closing connections with a code other than 1000, or in the range 3000-4999. --- packages/lib/link/connectors.js | 7 ++++++- packages/master/src/WsServer.js | 2 +- packages/master/src/WsServerConnector.js | 15 +++------------ 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/packages/lib/link/connectors.js b/packages/lib/link/connectors.js index e2aacdf7..dc7bc6a3 100644 --- a/packages/lib/link/connectors.js +++ b/packages/lib/link/connectors.js @@ -73,7 +73,12 @@ class WebSocketBaseConnector extends events.EventEmitter { this._check("connected"); if (Date.now() - this._lastHeartbeat > 2000 * this._heartbeatInterval) { logger.verbose("Connector | closing after heartbeat timed out"); - this._socket.close(1008, "Heartbeat timeout"); + // eslint-disable-next-line node/no-process-env + if (process.env.APP_ENV === "browser") { + this._socket.close(4008, "Heartbeat timeout"); + } else { + this._socket.terminate(); + } } else { this._socket.send(JSON.stringify({ diff --git a/packages/master/src/WsServer.js b/packages/master/src/WsServer.js index ea08fd91..1e1de286 100644 --- a/packages/master/src/WsServer.js +++ b/packages/master/src/WsServer.js @@ -136,7 +136,7 @@ class WsServer { let timeoutId = setTimeout(() => { logger.verbose(`WsServer | closing ${req.socket.remoteAddress} after timing out on handshake`); wsRejectedConnectionsCounter.inc(); - socket.close(1008, "Handshake timeout"); + socket.terminate(); this.pendingSockets.delete(socket); }, 30*1000); diff --git a/packages/master/src/WsServerConnector.js b/packages/master/src/WsServerConnector.js index bd58003d..1db846cb 100644 --- a/packages/master/src/WsServerConnector.js +++ b/packages/master/src/WsServerConnector.js @@ -73,19 +73,10 @@ class WsServerConnector extends libLink.WebSocketBaseConnector { continue(socket, lastSeq) { // It's possible the previous connection hasn't closed yet due to a - // stale connection. If this is the case the connector will have to - // wait for the previous socket to close down before it can continue - // from a new socket, or there will be overlapping events from both. + // stale connection. Terminate it if so. if (this._state === "connected") { - this._socket.close(4003, "Session Hijacked"); - - // Correctly waiting for the connector to close and then - // handling all the edge cases like multiple continues happening - // in parallel or connections going stale is far too difficult. - // Kill the connection here and let the client reconnect later - // when the connector is not held up by an active connection. - socket.close(1013, "Session Busy"); - socket.terminate(); + this._socket.terminate(); + this._socket.once("close", () => this.continue(socket, lastSeq)); return; } From bfec83996aae727267eee6c1eed27f0ca67bacd6 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Tue, 28 Sep 2021 21:28:38 +0200 Subject: [PATCH 06/14] Expect close in WsServer connection disconnect Don't bother doing the prepareDisconnect dance if the connection is already broken. This avoids having to wait until the connection times out when a slave gives up on a current connection and starts a new one. --- packages/master/src/BaseConnection.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/master/src/BaseConnection.js b/packages/master/src/BaseConnection.js index 44c086e6..e7bca6fa 100644 --- a/packages/master/src/BaseConnection.js +++ b/packages/master/src/BaseConnection.js @@ -64,6 +64,7 @@ class BaseConnection extends libLink.Link { } async disconnect(code, reason) { + this.connector.setClosing(); try { await libLink.messages.prepareDisconnect.send(this); } catch (err) { From 3b6ebeb653efe3f50eb74b11c9f9ffcd5559ea28 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Tue, 28 Sep 2021 21:32:29 +0200 Subject: [PATCH 07/14] Handle errors gathering metrics from slaves If the session to a slave is lost while gathering metrics the error would propagate out to the metrics route handler and break collection of statistics. Fix by handling errors thrown from sending the getMetrics request to slaves. --- packages/master/src/routes.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/master/src/routes.js b/packages/master/src/routes.js index 0310d001..1ab57577 100644 --- a/packages/master/src/routes.js +++ b/packages/master/src/routes.js @@ -8,6 +8,7 @@ const jwt = require("jsonwebtoken"); const path = require("path"); const util = require("util"); +const libErrors = require("@clusterio/lib/errors"); const libHelpers = require("@clusterio/lib/helpers"); const libLink = require("@clusterio/lib/link"); const libPlugin = require("@clusterio/lib/plugin"); @@ -47,7 +48,15 @@ async function getMetrics(req, res, next) { let requests = []; let timeout = req.app.locals.master.config.get("master.metrics_timeout") * 1000; for (let slaveConnection of req.app.locals.master.wsServer.slaveConnections.values()) { - requests.push(libHelpers.timeout(libLink.messages.getMetrics.send(slaveConnection), timeout, null)); + requests.push(libHelpers.timeout( + libLink.messages.getMetrics.send(slaveConnection).catch(err => { + if (!(err instanceof libErrors.SessionLost)) { + logger.error(`Unexpected error gathering metrics from slave:\n${err.stack}`); + } + return null; + }), + timeout, null + )); } for await (let result of await libPrometheus.defaultRegistry.collect()) { From e5582b5558fc5cd920b3bb9242743fe5d0fec065 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Tue, 28 Sep 2021 21:36:28 +0200 Subject: [PATCH 08/14] Skip gatherng metrics from dropped slaves Avoid stacking up huge amounts of metrics requests while a slave connection is in the dropped state. --- packages/master/src/routes.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/master/src/routes.js b/packages/master/src/routes.js index 1ab57577..749b2b55 100644 --- a/packages/master/src/routes.js +++ b/packages/master/src/routes.js @@ -48,6 +48,9 @@ async function getMetrics(req, res, next) { let requests = []; let timeout = req.app.locals.master.config.get("master.metrics_timeout") * 1000; for (let slaveConnection of req.app.locals.master.wsServer.slaveConnections.values()) { + if (!slaveConnection.connected) { + continue; + } requests.push(libHelpers.timeout( libLink.messages.getMetrics.send(slaveConnection).catch(err => { if (!(err instanceof libErrors.SessionLost)) { From e8b515c733ac1211a64d933a3edad995cc836847 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Tue, 28 Sep 2021 23:04:30 +0200 Subject: [PATCH 09/14] Update missing docs from connector refactor Fix documentation of max_reconnect_delay, heartbeat_interval and session_timeout which where changed with the connector refactor. --- docs/configuration.md | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index f78c609f..aab55e1f 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -86,19 +86,20 @@ Defaults to null which means generate a secure secret on startup. ### master.heartbeat_interval Interval in seconds heartbeats are sent out at for WebSocket connections. -If a connection hasn't received a heartbeat in 2 times the heartbeat interval it will be considered stale and closed. +If a WebSocket connection hasn't received a heartbeat in 2 times the heartbeat interval it will be considered stale and closed. A lower value means less time between connections going stale and them being closed. -Defaults to 45. +Defaults to 15. -### master.connector_shutdown_timeout +### master.session_timeout -Timeout in seconds to use while shutting down the master server for link sessions that currently don't have an active connection and haven't been closed down properly. -This happens when the network between a slave and the master server goes down unexpectedly, or the slave goes down without closing the connection properly either do to crashing or the machine it's hosted on going down. -If the timeout expires the session is terminated and data loss may occur, if the slave went down this is unavoidable. +Time in seconds from the connection is closed following a heartbeat timeout until the session is invalidated and data loss occurs. +A higher timeout gives more time for a client to recconect and resume an active session over a connection that was dropped, but also leads to stale connections taking longer to clear out. +In the case of the master or client crashing the data loss is unavoidable. -Defaults to 30. +Value should not be less than the configured `max_reconnect_delay` of the clients and it should be greater than 2 times `master.heartbeat_interval`. +Defaults to 60. ### master.metrics_timeout @@ -197,12 +198,12 @@ This is used by plugins like Server Select to give the correct address to switch Defaults to "localhost". -### slave.reconnect_delay +### slave.max_reconnect_delay -Delay in seconds to wait after connection to the master server is dropped before attempting to reconnect to it. -The actual delay on each reconnect will be a random number between 0 and this configured value to avoid all slaves trying to reconnect at the same time. +Maximum delay in seconds to wait after connection to the master server is dropped before attempting to reconnect to it. +The actual delay on each reconnect will be a random number between 0 and this configured value to avoid all clients trying to reconnect at the same time. -Defaults to 5. +Defaults to 60. ## Instance Configuration @@ -399,8 +400,9 @@ If you have a self signed certificate on the master server you will need to copy Defaults to null meaning use Node.js's default set of trusted certificate authorities. -### control.reconnect_delay +### control.max_reconnect_delay -Duration in seconds to wait before attempting to reconnect with the master server after the connection is dropped. +Max duration in seconds to wait before attempting to reconnect with the master server after the connection is dropped. +The actual delay on each reconnect will be a random number between 0 and this configured value to avoid all clients trying to reconnect at the same time. -Defaults to 2. +Defaults to 60. From c40362fa7fea39c910972d9d7de0093958c448bd Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Wed, 29 Sep 2021 00:09:21 +0200 Subject: [PATCH 10/14] Add sendOrderedRcon for plugins Add BaseInstancePlugin method for sending RCON commands in order to simplify logic in plugins that depend on commands being delivered in order. --- packages/lib/plugin.js | 68 +++++++++++++++++++++++++++++++++++++++++- test/lib/plugin.js | 32 +++++++++++++++++++- test/mock.js | 16 +++++++++- 3 files changed, 113 insertions(+), 3 deletions(-) diff --git a/packages/lib/plugin.js b/packages/lib/plugin.js index 834d2c96..fa438da5 100644 --- a/packages/lib/plugin.js +++ b/packages/lib/plugin.js @@ -60,6 +60,9 @@ class BaseInstancePlugin { * `error`, `warn`, `audit`, `info` and `verbose`. */ this.logger = instance.logger.child({ plugin: this.info.name }); + + this._pendingRconMessages = []; + this._sendingRconMessages = false; } /** @@ -207,9 +210,72 @@ class BaseInstancePlugin { */ async onPlayerEvent(event) { } - async sendRcon(message, expectEmpty) { + /** + * Send RCON message to instance + * + * Send a message or command to the server which can potentially be + * executed out of order in relation to other RCON commands. + * + * This should not be called before onStart or after onStop. A simple + * way to achieve this is to check that the instance status is running + * before sending commands. Note that the instance status is set to + * running some time after `onStart` and some time before `onStop` is + * invoked. + * + * @param {string} message - message to send to server over RCON. + * @param {boolean=} [expectEmpty=false] - + * if true throw if the response is not empty. Useful for detecting + * errors that might have been sent in response. + * @returns {Promise} response from server. + */ + async sendRcon(message, expectEmpty=false) { return await this.instance.sendRcon(message, expectEmpty, this.info.name); } + + /** + * Send serially ordered RCON message to instance + * + * Send a message or command to the server which will not be executed + * out of order in relation to other RCON commands sent with this + * method. The ordering applies per plugin, and two plugins sending + * commands with this method may execute out of order in relation to + * each other. + * + * This should not be called before onStart or after onStop. A simple + * way to achieve this is to check that the instance status is running + * before sending commands. Note that the instance status is set to + * running some time after `onStart` and some time before `onStop` is + * invoked. + * + * @param {string} message - message to send to server over RCON. + * @param {boolean=} [expectEmpty=false] - + * if true throw if the response is not empty. Useful for detecting + * errors that might have been sent in response. + * @returns {Promise} response from server. + */ + async sendOrderedRcon(message, expectEmpty=false) { + let promise = new Promise((resolve, reject) => { + this._pendingRconMessages.push({resolve, reject, message, expectEmpty}); + }); + if (!this._sendingRconMessages) { + this._sendPendingRconMessages(); + } + return await promise; + } + + async _sendPendingRconMessages() { + this._sendingRconMessages = true; + while (this._pendingRconMessages.length) { + let task = this._pendingRconMessages.shift(); + try { + let result = await this.sendRcon(task.message, task.expectEmpty); + task.resolve(result); + } catch (err) { + task.reject(err); + } + } + this._sendingRconMessages = false; + } } /** diff --git a/test/lib/plugin.js b/test/lib/plugin.js index 6ea1d9ac..55159f4f 100644 --- a/test/lib/plugin.js +++ b/test/lib/plugin.js @@ -10,7 +10,7 @@ describe("lib/plugin", function() { describe("class BaseInstancePlugin", function() { let instancePlugin; it("should be constructible", async function() { - instancePlugin = new libPlugin.BaseInstancePlugin({}, { logger: new mock.MockLogger() }, {}); + instancePlugin = new libPlugin.BaseInstancePlugin({}, new mock.MockInstance(), {}); await instancePlugin.init(); }); it("should define defaults for hooks", async function() { @@ -22,6 +22,36 @@ describe("lib/plugin", function() { instancePlugin.onMasterConnectionEvent("connect"); await instancePlugin.onPrepareMasterDisconnect(); }); + describe("sendRcon", function() { + it("should send commands out of order", async function() { + instancePlugin.instance.server.rconCommandResults.set("a", { time: 100, response: "a" }); + instancePlugin.instance.server.rconCommandResults.set("b", { time: 50, response: "b" }); + + let a = instancePlugin.sendRcon("a"); + let b = instancePlugin.sendRcon("b"); + let result = await Promise.race([a, b]); + assert.equal(result, "b"); + }); + it("should propagate errors", async function() { + instancePlugin.instance.server.rconCommandResults.set("a", new Error("ref")); + await assert.rejects(instancePlugin.sendRcon("a"), new Error("ref")); + }); + }); + describe("sendOrderedRcon", function() { + it("should send commands in order", async function() { + instancePlugin.instance.server.rconCommandResults.set("a", { time: 100, response: "a" }); + instancePlugin.instance.server.rconCommandResults.set("b", { time: 50, response: "b" }); + + let a = instancePlugin.sendOrderedRcon("a"); + let b = instancePlugin.sendOrderedRcon("b"); + let result = await Promise.race([a, b]); + assert.equal(result, "a"); + }); + it("should propagate errors", async function() { + instancePlugin.instance.server.rconCommandResults.set("a", new Error("ref")); + await assert.rejects(instancePlugin.sendOrderedRcon("a"), new Error("ref")); + }); + }); }); describe("class BaseMasterPlugin", function() { diff --git a/test/mock.js b/test/mock.js index 0787c403..4eec7794 100644 --- a/test/mock.js +++ b/test/mock.js @@ -3,6 +3,7 @@ const events = require("events"); const http = require("http"); const express = require("express"); +const libHelpers = require("@clusterio/lib/helpers"); const libLink = require("@clusterio/lib/link"); const libPlugin = require("@clusterio/lib/plugin"); const libUsers = require("@clusterio/lib/users"); @@ -105,7 +106,20 @@ class MockServer extends events.EventEmitter { async sendRcon(command) { this.rconCommands.push(command); - return this.rconCommandResults.get(command) || ""; + let result = this.rconCommandResults.get(command); + if (!result) { + result = { response: "" }; + } + if (result instanceof Error || typeof result === "string") { + result = { response: result }; + } + if (result.time) { + await libHelpers.wait(result.time); + } + if (result.response instanceof Error) { + throw result.response; + } + return result.response || ""; } } From 2b101d594aba872756b4c4921eddcd3e787974a9 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Wed, 29 Sep 2021 00:21:53 +0200 Subject: [PATCH 11/14] Fix statistics_exporter sending commands before start Fix statistics_exporter potentially sending commands before an instance is started when metrics are collected. --- plugins/statistics_exporter/instance.js | 4 ++++ plugins/statistics_exporter/test/plugin.js | 7 +++++++ test/mock.js | 1 + 3 files changed, 12 insertions(+) diff --git a/plugins/statistics_exporter/instance.js b/plugins/statistics_exporter/instance.js index db40dcbd..7d98f5eb 100644 --- a/plugins/statistics_exporter/instance.js +++ b/plugins/statistics_exporter/instance.js @@ -77,6 +77,10 @@ class InstancePlugin extends libPlugin.BaseInstancePlugin { } async onMetrics() { + if (this.instance.status !== "running") { + return; + } + // Wait configured timeout for the metrics to be collected. It may // take a long time for the command to go through if the command // stream is overloaded. Should the timeout be exceeded the diff --git a/plugins/statistics_exporter/test/plugin.js b/plugins/statistics_exporter/test/plugin.js index 50a28c88..56a16b5d 100644 --- a/plugins/statistics_exporter/test/plugin.js +++ b/plugins/statistics_exporter/test/plugin.js @@ -17,7 +17,14 @@ describe("statistics_exporter plugin", function() { }); describe(".onMetrics()", function() { + it("should not send commands before running", async function() { + instancePlugin.instance.server.rconCommands.length = 0; + instancePlugin.instance.status = "starting"; + await instancePlugin.onMetrics(); + assert(instancePlugin.instance.server.rconCommands.length === 0, "commands were run"); + }); it("should record statistics", async function() { + instancePlugin.instance.status = "running"; instancePlugin.instance.server.rconCommandResults.set( "/sc statistics_exporter.export()", JSON.stringify({ diff --git a/test/mock.js b/test/mock.js index 4eec7794..c66b3d0b 100644 --- a/test/mock.js +++ b/test/mock.js @@ -130,6 +130,7 @@ class MockInstance extends libLink.Link { this.server = new MockServer(); this.name = "test"; this.id = 7357; + this.status = "running"; this.mockConfigEntries = new Map([ ["instance.id", 7357], ["factorio.enable_save_patching", true], From 034ca4c2504f31e85c732410317ececf77f7ba83 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Wed, 29 Sep 2021 01:01:56 +0200 Subject: [PATCH 12/14] Fix research_sync sending commands before instance start Fix commands syncing researces being sent before the instance is started up, and potential desync of research progress as a result of out of order execution of RCON commands sent. --- plugins/research_sync/instance.js | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/plugins/research_sync/instance.js b/plugins/research_sync/instance.js index 0da6c858..3f58bf2c 100644 --- a/plugins/research_sync/instance.js +++ b/plugins/research_sync/instance.js @@ -19,6 +19,8 @@ class InstancePlugin extends libPlugin.BaseInstancePlugin { this.instance.server.on("ipc-research_sync:finished", (tech) => { this.researchFinished(tech).catch(err => this.unexpectedError(err)); }); + + this.syncStarted = false; } async researchContribution(tech) { @@ -26,8 +28,11 @@ class InstancePlugin extends libPlugin.BaseInstancePlugin { } async progressEventHandler(message) { + if (!this.syncStarted || !["starting", "running"].includes(this.instance.status)) { + return; + } let techsJson = libLuaTools.escapeString(JSON.stringify(message.data.technologies)); - await this.sendRcon(`/sc research_sync.update_progress("${techsJson}")`, true); + await this.sendOrderedRcon(`/sc research_sync.update_progress("${techsJson}")`, true); } async researchFinished(tech) { @@ -35,14 +40,17 @@ class InstancePlugin extends libPlugin.BaseInstancePlugin { } async finishedEventHandler(message) { + if (!this.syncStarted || !["starting", "running"].includes(this.instance.status)) { + return; + } let { name, level, researched } = message.data; - await this.sendRcon( + await this.sendOrderedRcon( `/sc research_sync.research_technology("${libLuaTools.escapeString(name)}", ${level})`, true ); } async onStart() { - let dumpJson = await this.sendRcon("/sc research_sync.dump_technologies()"); + let dumpJson = await this.sendOrderedRcon("/sc research_sync.dump_technologies()"); let techsToSend = []; let instanceTechs = new Map(); for (let tech of JSON.parse(dumpJson)) { @@ -56,6 +64,7 @@ class InstancePlugin extends libPlugin.BaseInstancePlugin { } let response = await this.info.messages.syncTechnologies.send(this.instance, { technologies: techsToSend }); + this.syncStarted = true; let techsToSync = []; for (let masterTech of response.technologies) { let [name, level, progress, researched] = masterTech; @@ -72,7 +81,7 @@ class InstancePlugin extends libPlugin.BaseInstancePlugin { if (techsToSync.length) { let syncJson = libLuaTools.escapeString(JSON.stringify(techsToSync)); - await this.sendRcon(`/sc research_sync.sync_technologies("${syncJson}")`, true); + await this.sendOrderedRcon(`/sc research_sync.sync_technologies("${syncJson}")`, true); } } } From bad86a5ff237e92a2778edb14bb6ba97b9c43ba4 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Wed, 29 Sep 2021 01:30:44 +0200 Subject: [PATCH 13/14] Fix inventory_sync uploading editor mode The serialization and deserialization of editor mode is not supported by the current code, and while the download correctly checked for the player being in editor mode, the upload did not check for the player being in editor mode causing players in editor mode to break. Fix by adding check in upload_inventory. --- plugins/inventory_sync/module/upload_inventory.lua | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/plugins/inventory_sync/module/upload_inventory.lua b/plugins/inventory_sync/module/upload_inventory.lua index d1c4cf23..5ab6051e 100644 --- a/plugins/inventory_sync/module/upload_inventory.lua +++ b/plugins/inventory_sync/module/upload_inventory.lua @@ -3,6 +3,18 @@ local serialize = require("modules/inventory_sync/serialize") local save_crafts = require("modules/inventory_sync/save_crafts") local function upload_inventory(player, player_record) + -- Editor mode is not supported. + if player.controller_type == defines.controllers.editor then + player.toggle_map_editor() + end + + if player.controller_type == defines.controllers.editor then + log("ERROR: Inventory sync failed to upload, unable to switch " .. player.name .. " out of editor mode") + player.print("ERROR: Inventory sync failed to upload, unable to switch out of editor mode") + global.inventory_sync.active_uploads[player.name] = nil + return + end + local serialized_player = serialize.serialize_player(player) serialized_player.generation = player_record.generation From 107284e7965f41bcb3b65d9022110a11bbc3e9c3 Mon Sep 17 00:00:00 2001 From: Hornwitser Date: Wed, 29 Sep 2021 20:03:38 +0200 Subject: [PATCH 14/14] Fix > and < being present in docs < and > must apparently not be escaped inside backticks. --- docs/devs/protocol.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/devs/protocol.md b/docs/devs/protocol.md index 39625378..d45957fe 100644 --- a/docs/devs/protocol.md +++ b/docs/devs/protocol.md @@ -99,7 +99,7 @@ The following messages are the only valid messages that can be sent during the h Sent by the master server when the WebSocket connection has been opened. - `version: string` - The version of the master, e.g. "2.0.0". -- `plugins: Object<string, string>` - +- `plugins: Object` - Object mapping plugin names to plugin version for plugins that are loaded on the master server. #### `register_slave` @@ -111,7 +111,7 @@ Slave handshake for establishing a new connection session. - `version: string` - The protocol version of the slave, e.g. "2.0.0". - `name: string` - Name of the slave. - `id: integer` - ID of the slave. -- `plugins: Object<string, string>` - +- `plugins: Object` - Object mapping plugin names to plugin version for plugins that are available on the slave. #### `register_control`