Merge pull request #625 from Cooldude2606/fix/loopback-routing

Fix loopback routing message and add tests

CHANGELOG.md

@@ -23,6 +23,20 @@ Many thanks to the following for contributing to this release:
 [@username](https://github.com/username)
 -->
 
+## Version 2.0.0-alpha.19
+
+### Changes
+
+- Fixed IPC errors reporting undefined when no stack trace was present [#624](https://github.com/clusterio/clusterio/pull/624).
+- Fixed loopback routing for instances reporting the wrong error message [#625](https://github.com/clusterio/clusterio/pull/625).
+
+### Breaking Changes
+
+- Previously silent errors for controller.sendEvent now throw exceptions [#625](https://github.com/clusterio/clusterio/pull/625).
+
+Many thanks to the following for contributing to this release:  
+[@Cooldude2606](https://github.com/Cooldude2606)
+
 ## Version 2.0.0-alpha.18
 
 ### Features

packages/controller/src/Controller.ts

@@ -1351,10 +1351,10 @@ export default class Controller {
 		throw Error(`Unknown type ${requestOrEvent.constructor.type}.`);
 	}
 
-	async sendRequest(request: any, dst: lib.Address) {
+	sendRequest(request: any, dst: lib.Address) {
 		let connection;
 		if (dst.type === lib.Address.controller) {
-			throw new Error("controller as dst is not supported.");
+			throw new Error(`Message would return back to sender ${dst}.`);
 
 		} else if (dst.type === lib.Address.control) {
 			connection = this.wsServer.controlConnections.get(dst.id);
@@ -1389,24 +1389,33 @@ export default class Controller {
 	sendEvent<T>(event: lib.Event<T>, dst: lib.Address) {
 		let connection;
 		if (dst.type === lib.Address.controller) {
-			throw new Error("controller as dst is not supported.");
+			throw new Error(`Message would return back to sender ${dst}.`);
 
 		} else if (dst.type === lib.Address.control) {
 			connection = this.wsServer.controlConnections.get(dst.id);
+			if (!connection) {
+				throw new Error("Target control connection does not exist.");
+			}
 
 		} else if (dst.type === lib.Address.instance) {
 			let instance = this.instances.get(dst.id);
 			if (!instance) {
-				return;
+				throw new Error(`Instance with ID ${dst.id} does not exist`);
 			}
 			let hostId = instance.config.get("instance.assigned_host");
 			if (hostId === null) {
-				return;
+				throw new Error("Instance is not assigned to a host");
 			}
 			connection = this.wsServer.hostConnections.get(hostId);
+			if (!connection) {
+				throw new Error("Host containing instance is not connected");
+			}
 
 		} else if (dst.type === lib.Address.host) {
 			connection = this.wsServer.hostConnections.get(dst.id);
+			if (!connection) {
+				throw new Error("Host is not connected");
+			}
 
 		} else if (dst.type === lib.Address.broadcast) {
 			if (dst.id === lib.Address.control) {
@@ -1424,7 +1433,7 @@ export default class Controller {
 				}
 
 			} else {
-				throw new Error(`Unexpected broacdast target ${dst.id}`);
+				throw new Error(`Unexpected broadcast target ${dst.id}`);
 			}
 			return;
 
@@ -1432,9 +1441,7 @@ export default class Controller {
 			throw new Error(`Unknown address type ${dst.type}`);
 		}
 
-		if (connection) {
-			connection.sendEvent(event, dst);
-		}
+		connection.sendEvent(event, dst);
 	}
 
 	async sendToHostByInstanceId<Req extends InstanceId, Res>(request: Req & lib.Request<Req, Res>): Promise<Res>;

packages/host/src/Host.ts

@@ -140,7 +140,7 @@ export class HostRouter {
 						return false;
 					}
 					origin.connector.sendResponseError(
-						new lib.ResponseError("Instance is not running."), message.src
+						new lib.ResponseError(msg || "Instance is not running."), message.src
 					);
 				}
 				return true;

packages/lib/src/link/link.ts

@@ -538,6 +538,9 @@ export class Link {
 		if (!entry) {
 			throw new Error(`Attempt to send unregistered Request ${request.constructor.name}`);
 		}
+		if (this.connector.src.equals(dst)) {
+			throw new Error(`Message would return back to sender ${dst}.`);
+		}
 		if (this.validateSent) {
 			entry.requestFromJSON(JSON.parse(JSON.stringify(request)));
 		}
@@ -572,6 +575,9 @@ export class Link {
 		if (!entry) {
 			throw new Error(`Attempt to send unregistered Event ${event.constructor.name}`);
 		}
+		if (this.connector.src.equals(dst)) {
+			throw new Error(`Message would return back to sender ${dst}.`);
+		}
 		if (this.validateSent) {
 			entry.eventFromJSON(JSON.parse(JSON.stringify(event)));
 		}

test/integration/index.js

@@ -189,6 +189,12 @@ before(async function() {
 		format: new lib.TerminalFormat(),
 	}));
 
+	// If fast test is enabled then output that it is
+	// eslint-disable-next-line node/no-process-env
+	if (process.env.FAST_TEST) {
+		console.log("FAST_TEST is present in env, slow tests will be skipped.");
+	}
+
 	await fs.remove(instancesDir);
 	await fs.remove(modsDir);
 	await fs.remove(databaseDir);

test/integration/routing.js

@@ -5,6 +5,9 @@ const lib = require("@clusterio/lib");
 const { Host, Instance, InstanceConnection } = require("@clusterio/host");
 const { ControlConnection, Controller, HostConnection, InstanceInfo } = require("@clusterio/controller");
 
+// Although there are no imports, the file has side-effects such as "before" which can effect tests
+require("./index");
+
 const addr = lib.Address.fromShorthand;
 
 class Control extends lib.Link { }
@@ -419,4 +422,30 @@ describe("Integration of link routing", function() {
 			});
 		}
 	});
+
+	describe("event should not be routable on loopback", function() {
+		for (const srcName of ["controller", "hostA", "instanceA1", "controlA"]) {
+			it(`should not loopback on ${srcName}`, async function() {
+				const src = get(srcName);
+				const dstAddr = src instanceof Controller ? addr("controller") : src.connector.src;
+				assert.throws(
+					() => send(src, dstAddr, new TestEvent()),
+					new Error(`Message would return back to sender ${dstAddr}.`)
+				);
+			});
+		}
+	});
+
+	describe("request should not be routable on loopback", function() {
+		for (const srcName of ["controller", "hostA", "instanceA1", "controlA"]) {
+			it(`should not loopback on ${srcName}`, async function() {
+				const src = get(srcName);
+				const dstAddr = src instanceof Controller ? addr("controller") : src.connector.src;
+				assert.throws(
+					() => send(src, dstAddr, new TestRequest()),
+					new Error(`Message would return back to sender ${dstAddr}.`)
+				);
+			});
+		}
+	});
 });