From 7e582d647a6e2fff7aba7431133638dcfd0fa6f9 Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=B4me=20Benoit?= Date: Thu, 24 Oct 2024 22:29:51 +0200 Subject: [PATCH] fix: ensure worker error is propagated unchanged if possible (#2634) MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit * fix: ensure worker error is propagated unchanged if possible closes #2633 Signed-off-by: Jérôme Benoit * refactor: cleanups Signed-off-by: Jérôme Benoit --------- Signed-off-by: Jérôme Benoit --- src/pools/abstract-pool.ts | 18 ++++++++++++------ src/utility-types.ts | 6 +++++- src/worker/abstract-worker.ts | 26 +++++++++++++------------- src/worker/cluster-worker.ts | 7 +++++++ src/worker/thread-worker.ts | 4 ++-- tests/pools/abstract-pool.test.mjs | 14 +++++++------- tests/pools/cluster/fixed.test.mjs | 10 ++++++++-- tests/pools/thread/fixed.test.mjs | 6 ++---- tests/worker/cluster-worker.test.mjs | 9 +++++---- tests/worker/thread-worker.test.mjs | 6 ++---- 10 files changed, 63 insertions(+), 43 deletions(-) diff --git a/src/pools/abstract-pool.ts b/src/pools/abstract-pool.ts index 72507108..ae4e3f10 100644 --- a/src/pools/abstract-pool.ts +++ b/src/pools/abstract-pool.ts @@ -1184,14 +1184,18 @@ export abstract class AbstractPool< const { asyncResource, reject, resolve, workerNodeKey } = promiseResponse const workerNode = this.workerNodes[workerNodeKey] if (workerError != null) { + let error: Error + if (workerError.error != null) { + error = workerError.error + } else { + const err = new Error(workerError.message) + err.stack = workerError.stack + error = err + } this.emitter?.emit(PoolEvents.taskError, workerError) asyncResource != null - ? asyncResource.runInAsyncScope( - reject, - this.emitter, - workerError.message - ) - : reject(workerError.message) + ? asyncResource.runInAsyncScope(reject, this.emitter, error) + : reject(error) } else { asyncResource != null ? asyncResource.runInAsyncScope(resolve, this.emitter, data) @@ -1493,6 +1497,7 @@ export abstract class AbstractPool< // eslint-disable-next-line @typescript-eslint/restrict-template-expressions `Task function operation '${message.taskFunctionOperation?.toString()}' failed on worker ${message.workerId?.toString()} with error: '${ // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + message.workerError?.error?.message ?? message.workerError?.message }'` ) @@ -1545,6 +1550,7 @@ export abstract class AbstractPool< // eslint-disable-next-line @typescript-eslint/restrict-template-expressions }' failed on worker ${errorResponse?.workerId?.toString()} with error: '${ // eslint-disable-next-line @typescript-eslint/restrict-template-expressions + errorResponse?.workerError?.error?.message ?? errorResponse?.workerError?.message }'` ) diff --git a/src/utility-types.ts b/src/utility-types.ts index 3c497457..c5caefc1 100644 --- a/src/utility-types.ts +++ b/src/utility-types.ts @@ -14,10 +14,14 @@ export interface WorkerError { * Data triggering the error. */ readonly data?: Data + /** + * Error object. + */ + readonly error?: Error /** * Error message. */ - readonly message: string + readonly message?: string /** * Task function name triggering the error. */ diff --git a/src/worker/abstract-worker.ts b/src/worker/abstract-worker.ts index 3b63c454..61a3e32f 100644 --- a/src/worker/abstract-worker.ts +++ b/src/worker/abstract-worker.ts @@ -85,9 +85,11 @@ export abstract class AbstractWorker< taskId, workerError: { data, - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - message: `Task function '${name!}' not found`, name, + ...this.handleError( + // eslint-disable-next-line @typescript-eslint/no-non-null-assertion + new Error(`Task function '${name!}' not found`) + ), }, }) return @@ -126,9 +128,8 @@ export abstract class AbstractWorker< taskId, workerError: { data, - message: this.handleErrorMessage(error as Error | string), name, - stack: (error as Error).stack, + ...this.handleError(error as Error), }, }) }) @@ -162,9 +163,8 @@ export abstract class AbstractWorker< taskId, workerError: { data, - message: this.handleErrorMessage(error as Error | string), name, - stack: (error as Error).stack, + ...this.handleError(error as Error), }, }) } finally { @@ -219,13 +219,14 @@ export abstract class AbstractWorker< } /** - * Handles an error and convert it if needed to its message string. - * Error are not structured-cloneable and cannot be sent to the main worker. + * Handles a worker error . * @param error - The error raised by the worker. - * @returns The error message. + * @returns The worker error object. */ - protected handleErrorMessage (error: Error | string): string { - return error instanceof Error ? error.message : error + protected abstract handleError (error: Error): { + error?: Error + message?: string + stack?: string } /** @@ -308,9 +309,8 @@ export abstract class AbstractWorker< ...(!status && error != null && { workerError: { - message: this.handleErrorMessage(error as Error | string), name: taskFunctionProperties.name, - stack: error.stack, + ...this.handleError(error), }, }), }) diff --git a/src/worker/cluster-worker.ts b/src/worker/cluster-worker.ts index e01e54e5..5a611e80 100644 --- a/src/worker/cluster-worker.ts +++ b/src/worker/cluster-worker.ts @@ -45,6 +45,13 @@ export class ClusterWorker< super(cluster.isPrimary, cluster.worker, taskFunctions, opts) } + /** + * @inheritDoc + */ + protected handleError (error: Error): { message: string; stack?: string } { + return { message: error.message, stack: error.stack } + } + /** @inheritDoc */ protected handleReadyMessage (message: MessageValue): void { if (message.workerId === this.id && message.ready === false) { diff --git a/src/worker/thread-worker.ts b/src/worker/thread-worker.ts index f1f9602f..4204d565 100644 --- a/src/worker/thread-worker.ts +++ b/src/worker/thread-worker.ts @@ -58,8 +58,8 @@ export class ThreadWorker< /** * @inheritDoc */ - protected override handleErrorMessage (error: Error | string): string { - return error as string + protected handleError (error: Error): { error: Error } { + return { error } } /** @inheritDoc */ diff --git a/tests/pools/abstract-pool.test.mjs b/tests/pools/abstract-pool.test.mjs index 777fff9f..d9d82f8a 100644 --- a/tests/pools/abstract-pool.test.mjs +++ b/tests/pools/abstract-pool.test.mjs @@ -959,8 +959,8 @@ describe('Abstract pool test suite', () => { await expect(pool.execute(undefined, undefined, {})).rejects.toThrow( new TypeError('transferList argument must be an array') ) - await expect(pool.execute(undefined, 'unknown')).rejects.toBe( - "Task function 'unknown' not found" + await expect(pool.execute(undefined, 'unknown')).rejects.toThrow( + new Error("Task function 'unknown' not found") ) await pool.destroy() await expect(pool.execute()).rejects.toThrow( @@ -1783,21 +1783,21 @@ describe('Abstract pool test suite', () => { const workerId = dynamicThreadPool.workerNodes[0].info.id await expect(dynamicThreadPool.setDefaultTaskFunction(0)).rejects.toThrow( new Error( - `Task function operation 'default' failed on worker ${workerId} with error: 'TypeError: name parameter is not a string'` + `Task function operation 'default' failed on worker ${workerId} with error: 'name parameter is not a string'` ) ) await expect( dynamicThreadPool.setDefaultTaskFunction(DEFAULT_TASK_NAME) ).rejects.toThrow( new Error( - `Task function operation 'default' failed on worker ${workerId} with error: 'Error: Cannot set the default task function reserved name as the default task function'` + `Task function operation 'default' failed on worker ${workerId} with error: 'Cannot set the default task function reserved name as the default task function'` ) ) await expect( dynamicThreadPool.setDefaultTaskFunction('unknown') ).rejects.toThrow( new Error( - `Task function operation 'default' failed on worker ${workerId} with error: 'Error: Cannot set the default task function to a non-existing task function'` + `Task function operation 'default' failed on worker ${workerId} with error: 'Cannot set the default task function to a non-existing task function'` ) ) expect(dynamicThreadPool.listTaskFunctionsProperties()).toStrictEqual([ @@ -1918,8 +1918,8 @@ describe('Abstract pool test suite', () => { await expect(pool.mapExecute([undefined], undefined, {})).rejects.toThrow( new TypeError('transferList argument must be an array') ) - await expect(pool.mapExecute([undefined], 'unknown')).rejects.toBe( - "Task function 'unknown' not found" + await expect(pool.mapExecute([undefined], 'unknown')).rejects.toThrow( + new Error("Task function 'unknown' not found") ) let results = await pool.mapExecute( [{}, {}, {}, {}], diff --git a/tests/pools/cluster/fixed.test.mjs b/tests/pools/cluster/fixed.test.mjs index 493399b3..748e9b2b 100644 --- a/tests/pools/cluster/fixed.test.mjs +++ b/tests/pools/cluster/fixed.test.mjs @@ -176,7 +176,9 @@ describe('Fixed cluster pool test suite', () => { } catch (e) { inError = e } - expect(inError).toStrictEqual('Error Message from ClusterWorker') + expect(inError).toBeInstanceOf(Error) + expect(inError.message).toStrictEqual('Error Message from ClusterWorker') + expect(typeof inError.stack === 'string').toBe(true) expect(taskError).toStrictEqual({ data, message: 'Error Message from ClusterWorker', @@ -206,7 +208,11 @@ describe('Fixed cluster pool test suite', () => { } catch (e) { inError = e } - expect(inError).toStrictEqual('Error Message from ClusterWorker:async') + expect(inError).toBeInstanceOf(Error) + expect(inError.message).toStrictEqual( + 'Error Message from ClusterWorker:async' + ) + expect(typeof inError.stack === 'string').toBe(true) expect(taskError).toStrictEqual({ data, message: 'Error Message from ClusterWorker:async', diff --git a/tests/pools/thread/fixed.test.mjs b/tests/pools/thread/fixed.test.mjs index 904d6a7f..8086bcfc 100644 --- a/tests/pools/thread/fixed.test.mjs +++ b/tests/pools/thread/fixed.test.mjs @@ -206,9 +206,8 @@ describe('Fixed thread pool test suite', () => { expect(inError.message).toStrictEqual('Error Message from ThreadWorker') expect(taskError).toStrictEqual({ data, - message: new Error('Error Message from ThreadWorker'), + error: new Error('Error Message from ThreadWorker'), name: DEFAULT_TASK_NAME, - stack: expect.any(String), }) expect( errorPool.workerNodes.some( @@ -239,9 +238,8 @@ describe('Fixed thread pool test suite', () => { ) expect(taskError).toStrictEqual({ data, - message: new Error('Error Message from ThreadWorker:async'), + error: new Error('Error Message from ThreadWorker:async'), name: DEFAULT_TASK_NAME, - stack: expect.any(String), }) expect( asyncErrorPool.workerNodes.some( diff --git a/tests/worker/cluster-worker.test.mjs b/tests/worker/cluster-worker.test.mjs index f76b0bfa..a7de4671 100644 --- a/tests/worker/cluster-worker.test.mjs +++ b/tests/worker/cluster-worker.test.mjs @@ -90,12 +90,13 @@ describe('Cluster worker test suite', () => { expect(worker.getMainWorker().send.calledOnce).toBe(true) }) - it('Verify that handleErrorMessage() method is working properly', () => { + it('Verify that handleError() method is working properly', () => { const error = new Error('Error as an error') const worker = new ClusterWorker(() => {}) - expect(worker.handleErrorMessage(error)).toStrictEqual(error.message) - const errorMessage = 'Error as a string' - expect(worker.handleErrorMessage(errorMessage)).toStrictEqual(errorMessage) + expect(worker.handleError(error)).toStrictEqual({ + message: error.message, + stack: error.stack, + }) }) it('Verify that sendToMainWorker() method invokes the getMainWorker() and send() methods', () => { diff --git a/tests/worker/thread-worker.test.mjs b/tests/worker/thread-worker.test.mjs index e3cd28da..2dbc9cb0 100644 --- a/tests/worker/thread-worker.test.mjs +++ b/tests/worker/thread-worker.test.mjs @@ -90,12 +90,10 @@ describe('Thread worker test suite', () => { expect(worker.port.postMessage.calledOnce).toBe(true) }) - it('Verify that handleErrorMessage() method is working properly', () => { + it('Verify that handleError() method is working properly', () => { const error = new Error('Error as an error') const worker = new ThreadWorker(() => {}) - expect(worker.handleErrorMessage(error)).toStrictEqual(error) - const errorMessage = 'Error as a string' - expect(worker.handleErrorMessage(errorMessage)).toStrictEqual(errorMessage) + expect(worker.handleError(error)).toStrictEqual({ error }) }) it('Verify that sendToMainWorker() method invokes the port property postMessage() method', () => { -- 2.34.1