Fix crash on race condition stopping Factorio server

If the server decides to exit on its own during the stop operation to
the FactorioServer class after the operation has started but before the
signal is sent to the process then the exit handler will have wiped the
server child process and cause the kill call to fail.  This in turn
would let the hang timer running which would bring down the Node.js
process due to an unhandled error for the same reason.

Fix by making sure the stop operation watches for the server exiting and
returning early if the server already quit before getting to the send
stop signal part.

packages/lib/factorio/server.js

@@ -522,6 +522,8 @@ class FactorioServer extends events.EventEmitter {
 	 * @param {number} options.maxConcurrentCommands -
 	 *     Maximum number of RCON commands transmitted in parallel on the
 	 *     RCON connection.
+	 * @param {number} options.hangTimeout -
+	 *     Timeout in ms to wait for a response to a shutdown command.
 	 */
 	constructor(factorioDir, writeDir, options) {
 		super();
@@ -547,6 +549,8 @@ class FactorioServer extends events.EventEmitter {
 		this.verboseLogging = options.verboseLogging;
 		/** Maximum number of RCON commands transmitted in parallel on the RCON connection  */
 		this.maxConcurrentCommands = options.maxConcurrentCommands || 5;
+		/** Timeout in ms to wait for a response to a shutdown command */
+		this.hangTimeout = options.hangTimeout || 5000;
 		this._state = "new";
 		this._server = null;
 		this._rconClient = null;
@@ -704,8 +708,6 @@ class FactorioServer extends events.EventEmitter {
 		this._rconClient.on("authenticated", () => { this._rconReady = true; this.emit("rcon-ready"); });
 		this._rconClient.on("end", () => {
 			this._rconClient = null;
-			this._rconReady = false;
-			this._startRcon().catch(() => { /* Ignore */ });
 		});
 		// XXX: Workaround to suppress bogus event listener warning
 		if (this._rconClient.emitter instanceof events.EventEmitter) {
@@ -802,6 +804,10 @@ class FactorioServer extends events.EventEmitter {
 				}
 			}
 
+			if (this._rconClient) {
+				this._rconClient.end().catch(() => {});
+			}
+
 			// Reset server state
 			this._state = "init";
 			this._server = null;
@@ -954,6 +960,9 @@ class FactorioServer extends events.EventEmitter {
 		if (!this._rconReady) {
 			await events.once(this, "rcon-ready");
 		}
+		if (!this._rconClient) {
+			throw new Error("RCON connection lost");
+		}
 
 		let response = await this._rconClient.send(message);
 		if (expectEmpty && response !== "") {
@@ -972,6 +981,7 @@ class FactorioServer extends events.EventEmitter {
 		this._check(["running"]);
 
 		let hanged = true;
+		let stopped = false;
 		function setAlive() { hanged = false; }
 
 		let timeoutId = setTimeout(() => {
@@ -982,7 +992,14 @@ class FactorioServer extends events.EventEmitter {
 				}
 				this._server.kill("SIGKILL");
 			}
-		}, 5000);
+		}, this.hangTimeout);
+
+		// It's possible the server decides to exit on its own, in which
+		// case the stop operation has to be cancelled.
+		this._server.once("exit", () => {
+			stopped = true;
+			clearTimeout(timeoutId);
+		});
 
 		this._state = "stopping";
 		// On Windows we need to save the map as there's no graceful shutdown.
@@ -1014,6 +1031,12 @@ class FactorioServer extends events.EventEmitter {
 			await this._rconClient.end();
 		}
 
+		// The Factorio server may have decided to get ahead of us and
+		// stop by itself before we had the chance to signal it.
+		if (stopped) {
+			return;
+		}
+
 		// On linux this sends SIGTERM to the process.  On windows this
 		// will terminate the process with no cleanup.  You can send your
 		// complaint for the lack of graceful shutdown to the Factorio

test/lib/factorio/server.js

@@ -5,6 +5,7 @@ const fs = require("fs-extra");
 const path = require("path");
 
 const libFactorio = require("@clusterio/lib/factorio");
+const { wait } = require("@clusterio/lib/helpers");
 const { testLines } = require("./lines");
 
 
@@ -243,5 +244,31 @@ describe("lib/factorio/server", function() {
 				assert(!await fs.pathExists(filePath), "File was not deleted");
 			});
 		});
+
+		describe(".stop()", function() {
+			it("should handle server quitting on its own during stop", async function() {
+				if (process.platform === "win32") {
+					this.skip();
+				}
+				server.hangTimeout = 20;
+				server._server = new events.EventEmitter();
+				server._server.kill = () => true;
+				server._state = "running";
+				server._rconReady = true;
+				server._rconClient = {
+					async end() {
+						server._rconClient = null;
+						process.nextTick(() => {
+							server.emit("_quitting");
+							server._server.emit("exit");
+						});
+					},
+				};
+				server._watchExit();
+
+				await server.stop();
+				await wait(21); // Wait until after hang timeout
+			});
+		});
 	});
 });