Error handler doesn't catch the errors in localEvent hook. Closes #991 …

…#996
moleculerjs · Jan 14, 2023 · 317e68b · 317e68b
1 parent 096d0af
commit 317e68b
Show file tree

Hide file tree

Showing 8 changed files with 48 additions and 63 deletions.
diff --git a/src/middlewares/error-handler.js b/src/middlewares/error-handler.js
@@ -44,33 +44,28 @@ function wrapActionErrorHandler(handler) {
 function wrapEventErrorHandler(handler) {
 	return function errorHandlerMiddleware(ctx) {
 		// Call the handler
-		return handler(ctx)
-			.catch(err => {
-				if (!(err instanceof Error)) err = new MoleculerError(err, 500);
+		return handler(ctx).catch(err => {
+			if (!(err instanceof Error)) err = new MoleculerError(err, 500);
 
-				this.logger.debug(
-					`Error occured in the '${ctx.event.name}' event handler in the '${ctx.service.fullName}' service.`,
-					{ requestID: ctx.requestID },
-					err
-				);
+			this.logger.debug(
+				`Error occured in the '${ctx.event.name}' event handler in the '${ctx.service.fullName}' service.`,
+				{ requestID: ctx.requestID },
+				err
+			);
 
-				Object.defineProperty(err, "ctx", {
-					value: ctx,
-					writable: true,
-					enumerable: false
-				});
+			Object.defineProperty(err, "ctx", {
+				value: ctx,
+				writable: true,
+				enumerable: false
+			});
 
-				// Call global errorHandler
-				return ctx.broker.errorHandler(err, {
-					ctx,
-					service: ctx.service,
-					event: ctx.event
-				});
-			})
-			.catch(err => {
-				// No global error Handler, or thrown further, so we handle it because it's an event handler.
-				ctx.broker.logger.error(err);
+			// Call global errorHandler
+			return ctx.broker.errorHandler(err, {
+				ctx,
+				service: ctx.service,
+				event: ctx.event
 			});
+		});
 	}.bind(this);
 }
 

diff --git a/src/registry/action-catalog.js b/src/registry/action-catalog.js
@@ -133,7 +133,7 @@ class ActionCatalog {
 		onlyAvailable = false,
 		skipInternal = false,
 		withEndpoints = false
-	}) {
+	} = {}) {
 		let res = [];
 
 		this.actions.forEach((list, key) => {

diff --git a/src/registry/event-catalog.js b/src/registry/event-catalog.js
@@ -1,6 +1,6 @@
 /*
  * moleculer
- * Copyright (c) 2018 MoleculerJS (https://github.com/moleculerjs/moleculer)
+ * Copyright (c) 2023 MoleculerJS (https://github.com/moleculerjs/moleculer)
  * MIT Licensed
  */
 
@@ -246,7 +246,7 @@ class EventCatalog {
 		onlyAvailable = false,
 		skipInternal = false,
 		withEndpoints = false
-	}) {
+	} = {}) {
 		let res = [];
 
 		this.events.forEach(list => {

diff --git a/src/registry/node-catalog.js b/src/registry/node-catalog.js
@@ -206,7 +206,7 @@ class NodeCatalog {
 	 * @returns
 	 * @memberof NodeCatalog
 	 */
-	list({ onlyAvailable = false, withServices = false }) {
+	list({ onlyAvailable = false, withServices = false } = {}) {
 		let res = [];
 		this.nodes.forEach(node => {
 			if (onlyAvailable && !node.available) return;

diff --git a/src/registry/service-catalog.js b/src/registry/service-catalog.js
@@ -87,7 +87,7 @@ class ServiceCatalog {
 		withActions = false,
 		withEvents = false,
 		grouping = false
-	}) {
+	} = {}) {
 		let res = [];
 		this.services.forEach(service => {
 			if (skipInternal && /^\$/.test(service.name)) return;

diff --git a/src/service-broker.js b/src/service-broker.js
@@ -1,6 +1,6 @@
 /*
  * moleculer
- * Copyright (c) 2020 MoleculerJS (https://github.com/moleculerjs/moleculer)
+ * Copyright (c) 2023 MoleculerJS (https://github.com/moleculerjs/moleculer)
  * MIT Licensed
  */
 
@@ -1425,7 +1425,12 @@ class ServiceBroker {
 				if (ep.id === this.nodeID) {
 					// Local service, call handler
 					const newCtx = ctx.copy(ep);
-					promises.push(this.registry.events.callEventHandler(newCtx));
+					promises.push(
+						this.registry.events.callEventHandler(newCtx).catch(err => {
+							// Catch and log the error because it's a local event handler, not throwing further.
+							this.logger.error(err);
+						})
+					);
 				} else {
 					// Remote service
 					const e = groupedEP[ep.id];
@@ -1564,7 +1569,10 @@ class ServiceBroker {
 		ctx.eventType = "broadcastLocal";
 		ctx.eventGroups = opts.groups;
 
-		return this.emitLocalServices(ctx);
+		return this.emitLocalServices(ctx).catch(err => {
+			// Catch and log the error because it's a local event handler, not throwing further.
+			this.logger.error(err);
+		});
 	}
 
 	/**
@@ -1698,7 +1706,10 @@ class ServiceBroker {
 	 * @memberof ServiceBroker
 	 */
 	emitLocalServices(ctx) {
-		return this.registry.events.emitLocalServices(ctx);
+		return this.registry.events.emitLocalServices(ctx).catch(err => {
+			// Catch and log the error because it's a local event handler, not throwing further.
+			this.logger.error(err);
+		});
 	}
 
 	/**

diff --git a/test/unit/middlewares/error-handler.spec.js b/test/unit/middlewares/error-handler.spec.js
@@ -143,7 +143,9 @@ describe("Test ErrorHandlerMiddleware", () => {
 			const ctx = Context.create(broker, eventEndpoint);
 
 			return newHandler(ctx)
-				.catch(protectReject)
+				.catch(err => {
+					expect(err).toBe(error);
+				})
 				.then(() => {
 					expect(error.ctx).toBe(ctx);
 					expect(broker.errorHandler).toBeCalledTimes(1);
@@ -163,7 +165,9 @@ describe("Test ErrorHandlerMiddleware", () => {
 			const ctx = Context.create(broker, eventEndpoint);
 
 			return newHandler(ctx)
-				.catch(protectReject)
+				.catch(err => {
+					expect(err).toBeInstanceOf(MoleculerError);
+				})
 				.then(() => {
 					const err = broker.errorHandler.mock.calls[0][0];
 
@@ -181,30 +185,5 @@ describe("Test ErrorHandlerMiddleware", () => {
 					});
 				});
 		});
-
-		it("should logging if throw further", () => {
-			broker.errorHandler = jest.fn(err => Promise.reject(err));
-			let error = new MoleculerError("Some error");
-			let handler = jest.fn(() => Promise.reject(error));
-
-			const newHandler = mw.localEvent.call(broker, handler, action);
-			const ctx = Context.create(broker, eventEndpoint);
-			ctx.id = "123456";
-			ctx.nodeID = "server-2";
-
-			jest.spyOn(broker.logger, "error");
-
-			return newHandler(ctx)
-				.catch(protectReject)
-				.then(() => {
-					const err = broker.errorHandler.mock.calls[0][0];
-
-					expect(err).toBe(error);
-					expect(err.ctx).toBe(ctx);
-
-					expect(broker.logger.error).toHaveBeenCalledTimes(1);
-					expect(broker.logger.error).toHaveBeenCalledWith(err);
-				});
-		});
 	});
 });
diff --git a/test/unit/service-broker.spec.js b/test/unit/service-broker.spec.js
@@ -2780,7 +2780,7 @@ describe("Test broker.emit", () => {
 		]
 	]);
 	broker.localBus.emit = jest.fn();
-	broker.registry.events.callEventHandler = jest.fn();
+	broker.registry.events.callEventHandler = jest.fn(() => Promise.resolve());
 
 	it("should call the local handler", () => {
 		expect(broker.transit).toBeUndefined();
@@ -3066,7 +3066,7 @@ describe("Test broker.emit with transporter", () => {
 		]
 	]);
 	broker.localBus.emit = jest.fn();
-	broker.registry.events.callEventHandler = jest.fn();
+	broker.registry.events.callEventHandler = jest.fn(() => Promise.resolve());
 	broker.getEventGroups = jest.fn(() => ["mail", "payment"]);
 
 	it("should call sendEvent with ctx", () => {
@@ -3634,7 +3634,7 @@ describe("Test broker broadcast", () => {
 
 describe("Test broker broadcastLocal", () => {
 	let broker = new ServiceBroker({ logger: false, nodeID: "server-1" });
-	broker.emitLocalServices = jest.fn();
+	broker.emitLocalServices = jest.fn(() => Promise.resolve());
 	broker.localBus.emit = jest.fn();
 
 	it("should call emitLocalServices without payload", () => {
@@ -3897,7 +3897,7 @@ describe("Test registry links", () => {
 	broker.registry.getLocalNodeInfo = jest.fn();
 	broker.registry.events.getGroups = jest.fn();
 	broker.registry.events.getAllEndpoints = jest.fn(() => [{}]);
-	broker.registry.events.emitLocalServices = jest.fn();
+	broker.registry.events.emitLocalServices = jest.fn(() => Promise.resolve());
 
 	it("should call registry.getLocalNodeInfo", () => {
 		broker.getLocalNodeInfo();