catch unhandled promise rejection in event handlers. Fix 403 again

moleculerjs · Nov 4, 2018 · 7a4fc77 · 7a4fc77
1 parent 8e31f65
commit 7a4fc77
Show file tree

Hide file tree

Showing 5 changed files with 191 additions and 65 deletions.
diff --git a/dev/event-catch.js b/dev/event-catch.js
@@ -0,0 +1,63 @@
+const { ServiceBroker } = require("..");
+
+const ErrorMiddleware = {
+	localEvent(next) {
+		return async (payload) => {
+			try {
+				console.log("Begin localEvent");
+				await next(payload);
+				console.log("End localEvent");
+			} catch (e) {
+				console.log("Catched:", e);
+				throw e;
+			}
+		};
+	}
+};
+
+const broker = new ServiceBroker({
+	nodeID: "broker1",
+	middlewares: [
+		ErrorMiddleware
+	],
+	//transporter: "NATS"
+});
+/*
+con/st broker2 = new ServiceBroker({
+	nodeID: "broker2",
+	transporter: "NATS"
+});*/
+
+const sleep = ms => new Promise(res => setTimeout(res, ms));
+
+broker.createService({
+	name: "my",
+	events: {
+		yo: [
+			async function (payload) {
+				console.log("1 Begin yo", payload);
+				await sleep(1000);
+				console.log("1 End yo", payload);
+				//throw new Error("1 Capture me");
+			},
+			async function (payload) {
+				console.log("2 Begin yo", payload);
+				await sleep(1000);
+				console.log("2 End yo", payload);
+				throw new Error("2 Capture me");
+			}
+		]
+	}
+});
+
+(async function() {
+	await broker.start();
+	//await broker2.start();
+
+	await sleep(1000);
+	broker2.emit("yo", "world");
+	// broker.broadcast("yo", "world");
+	// broker.broadcastLocal("yo", "world");
+
+	//broker2.repl();
+})();
diff --git a/src/registry/event-catalog.js b/src/registry/event-catalog.js
@@ -149,17 +149,34 @@ class EventCatalog {
 				if (broadcast) {
 					list.endpoints.forEach(ep => {
 						if (ep.local && ep.event.handler)
-							ep.event.handler(payload, nodeID, eventName);
+							this.callEventHandler(ep.event.handler, payload, nodeID, eventName);
 					});
 				} else {
 					const ep = list.nextLocal();
 					if (ep && ep.event.handler)
-						ep.event.handler(payload, nodeID, eventName);
+						this.callEventHandler(ep.event.handler, payload, nodeID, eventName);
 				}
 			}
 		});
 	}
 
+	/**
+	 * Call local event handler and handles unhandled promise rejections.
+	 *
+	 * @param {Function} handler
+	 * @param {any} payload
+	 * @param {String} sender
+	 * @param {String} eventName
+	 *
+	 * @memberof EventCatalog
+	 */
+	callEventHandler(handler, payload, sender, eventName) {
+		const res = handler(payload, sender, eventName);
+		if (utils.isPromise(res))
+			return res.catch(err => this.broker.logger.error(err));
+		return res;
+	}
+
 	/**
 	 * Remove endpoints by service
 	 *

diff --git a/src/service-broker.js b/src/service-broker.js
@@ -1014,6 +1014,7 @@ class ServiceBroker {
 		}
 	}
 
+
 	/**
 	 * Emit an event (grouped & balanced global event)
 	 *
@@ -1045,7 +1046,7 @@ class ServiceBroker {
 				if (ep) {
 					if (ep.id == this.nodeID) {
 						// Local service, call handler
-						ep.event.handler(payload, this.nodeID, eventName);
+						this.registry.events.callEventHandler(ep.event.handler, payload, this.nodeID, eventName);
 					} else {
 						// Remote service
 						const e = groupedEP[ep.id];

diff --git a/test/unit/registry/event-catalog.spec.js b/test/unit/registry/event-catalog.spec.js
@@ -5,6 +5,7 @@ let EventCatalog = require("../../../src/registry/event-catalog");
 let EndpointList = require("../../../src/registry/endpoint-list");
 let EventEndpoint = require("../../../src/registry/endpoint-event");
 let ServiceBroker = require("../../../src/service-broker");
+const { protectReject } = require("../utils");
 
 describe("Test EventCatalog constructor", () => {
 
@@ -311,6 +312,8 @@ describe("Test EventCatalog.emitLocalServices", () => {
 	let broker = new ServiceBroker({ logger: false, nodeID: "node-1" });
 	let catalog = new EventCatalog(broker.registry, broker, Strategy);
 
+	catalog.callEventHandler = jest.fn();
+
 	let usersEvent = { name: "user.created", handler: jest.fn() };
 	let paymentEvent = { name: "user.created", handler: jest.fn() };
 	let mailEvent = { name: "user.*", handler: jest.fn() };
@@ -327,69 +330,102 @@ describe("Test EventCatalog.emitLocalServices", () => {
 		let payload = { a: 5 };
 		catalog.emitLocalServices("user.created", payload, null, "node-99", true);
 
-		expect(usersEvent.handler).toHaveBeenCalledTimes(1);
-		expect(usersEvent.handler).toHaveBeenCalledWith(payload, "node-99", "user.created");
-		expect(paymentEvent.handler).toHaveBeenCalledTimes(1);
-		expect(paymentEvent.handler).toHaveBeenCalledWith(payload, "node-99", "user.created");
-		expect(mailEvent.handler).toHaveBeenCalledTimes(1);
-		expect(mailEvent.handler).toHaveBeenCalledWith(payload, "node-99", "user.created");
-		expect(otherEvent.handler).toHaveBeenCalledTimes(1);
-		expect(otherEvent.handler).toHaveBeenCalledWith(payload, "node-99", "user.created");
-
+		expect(catalog.callEventHandler).toHaveBeenCalledTimes(4);
+		expect(catalog.callEventHandler).toHaveBeenCalledWith(usersEvent.handler, payload, "node-99", "user.created");
+		expect(catalog.callEventHandler).toHaveBeenCalledWith(paymentEvent.handler, payload, "node-99", "user.created");
+		expect(catalog.callEventHandler).toHaveBeenCalledWith(otherEvent.handler, payload, "node-99", "user.created");
+		expect(catalog.callEventHandler).toHaveBeenCalledWith(mailEvent.handler, payload, "node-99", "user.created");
 	});
 
 	it("should broadcast local handlers with groups", () => {
-		usersEvent.handler.mockClear();
-		paymentEvent.handler.mockClear();
-		mailEvent.handler.mockClear();
-		otherEvent.handler.mockClear();
+		catalog.callEventHandler.mockClear();
 
 		let payload = { a: 5 };
 		catalog.emitLocalServices("user.created", payload, ["mail", "payment"], "node-99", true);
 
-		expect(usersEvent.handler).toHaveBeenCalledTimes(0);
-		expect(paymentEvent.handler).toHaveBeenCalledTimes(1);
-		expect(paymentEvent.handler).toHaveBeenCalledWith(payload, "node-99", "user.created");
-		expect(mailEvent.handler).toHaveBeenCalledTimes(1);
-		expect(mailEvent.handler).toHaveBeenCalledWith(payload, "node-99", "user.created");
-		expect(otherEvent.handler).toHaveBeenCalledTimes(1);
-		expect(otherEvent.handler).toHaveBeenCalledWith(payload, "node-99", "user.created");
-
+		expect(catalog.callEventHandler).toHaveBeenCalledTimes(3);
+		expect(catalog.callEventHandler).toHaveBeenCalledWith(paymentEvent.handler, payload, "node-99", "user.created");
+		expect(catalog.callEventHandler).toHaveBeenCalledWith(otherEvent.handler, payload, "node-99", "user.created");
+		expect(catalog.callEventHandler).toHaveBeenCalledWith(mailEvent.handler, payload, "node-99", "user.created");
 	});
 
 	it("should balance local handlers without groups", () => {
-		usersEvent.handler.mockClear();
-		paymentEvent.handler.mockClear();
-		mailEvent.handler.mockClear();
-		otherEvent.handler.mockClear();
+		catalog.callEventHandler.mockClear();
 
 		let payload = { a: 5 };
 		catalog.emitLocalServices("user.created", payload, null, "node-99", false);
 
-		expect(usersEvent.handler).toHaveBeenCalledTimes(1);
-		expect(usersEvent.handler).toHaveBeenCalledWith(payload, "node-99", "user.created");
-		expect(paymentEvent.handler).toHaveBeenCalledTimes(1);
-		expect(paymentEvent.handler).toHaveBeenCalledWith(payload, "node-99", "user.created");
-		expect(mailEvent.handler).toHaveBeenCalledTimes(1);
-		expect(mailEvent.handler).toHaveBeenCalledWith(payload, "node-99", "user.created");
-		expect(otherEvent.handler).toHaveBeenCalledTimes(0); // Balanced
+		expect(catalog.callEventHandler).toHaveBeenCalledTimes(3);
+		expect(catalog.callEventHandler).toHaveBeenCalledWith(paymentEvent.handler, payload, "node-99", "user.created");
+		expect(catalog.callEventHandler).toHaveBeenCalledWith(usersEvent.handler, payload, "node-99", "user.created");
+		expect(catalog.callEventHandler).toHaveBeenCalledWith(mailEvent.handler, payload, "node-99", "user.created");
+
 	});
 
 	it("should balance local handlers with groups", () => {
-		usersEvent.handler.mockClear();
-		paymentEvent.handler.mockClear();
-		mailEvent.handler.mockClear();
-		otherEvent.handler.mockClear();
+		catalog.callEventHandler.mockClear();
 
 		let payload = { a: 5 };
 		catalog.emitLocalServices("user.created", payload, ["mail", "payment"], "node-99", false);
 
-		expect(usersEvent.handler).toHaveBeenCalledTimes(0);
-		expect(paymentEvent.handler).toHaveBeenCalledTimes(0); // Balanced
-		expect(mailEvent.handler).toHaveBeenCalledTimes(1);
-		expect(mailEvent.handler).toHaveBeenCalledWith(payload, "node-99", "user.created");
-		expect(otherEvent.handler).toHaveBeenCalledTimes(1);
-		expect(otherEvent.handler).toHaveBeenCalledWith(payload, "node-99", "user.created");
+		expect(catalog.callEventHandler).toHaveBeenCalledTimes(2);
+		expect(catalog.callEventHandler).toHaveBeenCalledWith(otherEvent.handler, payload, "node-99", "user.created");
+		expect(catalog.callEventHandler).toHaveBeenCalledWith(mailEvent.handler, payload, "node-99", "user.created");
+
+	});
+});
+
+describe("Test EventCatalog.callEventHandler", () => {
+	let broker = new ServiceBroker({ logger: false, nodeID: "node-1" });
+	let catalog = new EventCatalog(broker.registry, broker, Strategy);
+
+	it("should add catch handler to result", () => {
+		let payload = { a: 5 };
+
+		let resolver;
+		const handler = jest.fn(() => new Promise(res => resolver = res));
+
+		const p = catalog.callEventHandler(handler, payload, "node-99", "user.created");
+
+		expect(handler).toHaveBeenCalledTimes(1);
+		expect(handler).toHaveBeenCalledWith(payload, "node-99", "user.created");
+
+		resolver();
+
+		return p;
+	});
+
+	it("should catch error", () => {
+		let payload = { a: 5 };
+
+		let rejecter;
+		const handler = jest.fn(() => new Promise((res, rej) => rejecter = rej));
+		broker.logger.error = jest.fn();
+
+		const p = catalog.callEventHandler(handler, payload, "node-99", "user.created");
+
+		expect(handler).toHaveBeenCalledTimes(1);
+		expect(handler).toHaveBeenCalledWith(payload, "node-99", "user.created");
+
+		const err = new Error("Something went wrong");
+		rejecter(err);
+
+		return p.catch(protectReject).then(() => {
+			expect(broker.logger.error).toHaveBeenCalledTimes(1);
+			expect(broker.logger.error).toHaveBeenCalledWith(err);
+		});
+
+	});
+
+	it("should do nothing if result is not Promise", () => {
+		let payload = { a: 5 };
+
+		const handler = jest.fn(() => 5);
+		const res = catalog.callEventHandler(handler, payload, "node-99", "user.created");
+
+		expect(res).toBe(5);
+		expect(handler).toHaveBeenCalledTimes(1);
+		expect(handler).toHaveBeenCalledWith(payload, "node-99", "user.created");
 	});
 });