From: Jérôme Benoit Date: Wed, 20 Aug 2025 23:25:51 +0000 (+0200) Subject: fix: properly account strategy retries on a per strategy basis X-Git-Tag: v5.1.3~5 X-Git-Url: https://git.piment-noir.org/?a=commitdiff_plain;h=910c16976ee437c1b9011b6db197386a21c8739f;p=poolifier.git fix: properly account strategy retries on a per strategy basis Signed-off-by: Jérôme Benoit --- diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 066d9224f..cadec2e33 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1989,8 +1989,8 @@ packages: engines: {node: '>=20.17'} hasBin: true - listr2@9.0.1: - resolution: {integrity: sha512-SL0JY3DaxylDuo/MecFeiC+7pedM0zia33zl0vcjgwcq1q1FWWF1To9EIauPbl8GbMCU0R2e0uJ8bZunhYKD2g==} + listr2@9.0.2: + resolution: {integrity: sha512-VVd7cS6W+vLJu2wmq4QmfVj14Iep7cz4r/OWNk36Aq5ZOY7G8/BfCrQFexcwB1OIxB3yERiePfE/REBjEFulag==} engines: {node: '>=20.0.0'} locate-path@6.0.0: @@ -4848,7 +4848,7 @@ snapshots: commander: 14.0.0 debug: 4.4.1(supports-color@8.1.1) lilconfig: 3.1.3 - listr2: 9.0.1 + listr2: 9.0.2 micromatch: 4.0.8 nano-spawn: 1.0.2 pidtree: 0.6.0 @@ -4857,7 +4857,7 @@ snapshots: transitivePeerDependencies: - supports-color - listr2@9.0.1: + listr2@9.0.2: dependencies: cli-truncate: 4.0.0 colorette: 2.0.20 diff --git a/src/pools/abstract-pool.ts b/src/pools/abstract-pool.ts index 332656a00..d074b72ed 100644 --- a/src/pools/abstract-pool.ts +++ b/src/pools/abstract-pool.ts @@ -99,7 +99,8 @@ export abstract class AbstractPool< minSize: this.minimumNumberOfWorkers, ready: this.ready, started: this.started, - strategyRetries: this.workerChoiceStrategiesContext?.retriesCount ?? 0, + strategyRetries: + this.workerChoiceStrategiesContext?.getStrategyRetries() ?? 0, type: this.type, version, worker: this.worker, diff --git a/src/pools/selection-strategies/abstract-worker-choice-strategy.ts b/src/pools/selection-strategies/abstract-worker-choice-strategy.ts index ed8af7607..6714934ab 100644 --- a/src/pools/selection-strategies/abstract-worker-choice-strategy.ts +++ b/src/pools/selection-strategies/abstract-worker-choice-strategy.ts @@ -28,6 +28,9 @@ export abstract class AbstractWorkerChoiceStrategy< /** @inheritDoc */ public abstract readonly name: WorkerChoiceStrategy + /** @inheritDoc */ + public retriesCount: number + /** @inheritDoc */ public readonly strategyPolicy: StrategyPolicy = Object.freeze({ dynamicWorkerReady: true, @@ -45,12 +48,12 @@ export abstract class AbstractWorkerChoiceStrategy< /** * The next worker node key. */ - protected nextWorkerNodeKey: number | undefined = 0 + protected nextWorkerNodeKey: number | undefined /** * The previous worker node key. */ - protected previousWorkerNodeKey = 0 + protected previousWorkerNodeKey: number /** * Constructs a worker choice strategy bound to the pool. @@ -61,6 +64,9 @@ export abstract class AbstractWorkerChoiceStrategy< protected readonly pool: IPool, protected opts?: WorkerChoiceStrategyOptions ) { + this.retriesCount = 0 + this.nextWorkerNodeKey = 0 + this.previousWorkerNodeKey = 0 this.choose = this.choose.bind(this) this.setOptions(this.opts) } diff --git a/src/pools/selection-strategies/selection-strategies-types.ts b/src/pools/selection-strategies/selection-strategies-types.ts index 19a0e2215..2947c9824 100644 --- a/src/pools/selection-strategies/selection-strategies-types.ts +++ b/src/pools/selection-strategies/selection-strategies-types.ts @@ -86,6 +86,10 @@ export interface IWorkerChoiceStrategy { * @returns `true` if the reset is successful, `false` otherwise. */ readonly reset: () => boolean + /** + * The worker choice strategy execution retries count. + */ + retriesCount: number /** * Sets the worker choice strategy options. * @param opts - The worker choice strategy options. diff --git a/src/pools/selection-strategies/worker-choice-strategies-context.ts b/src/pools/selection-strategies/worker-choice-strategies-context.ts index 41d2fef89..5bb425509 100644 --- a/src/pools/selection-strategies/worker-choice-strategies-context.ts +++ b/src/pools/selection-strategies/worker-choice-strategies-context.ts @@ -28,11 +28,6 @@ export class WorkerChoiceStrategiesContext< Data = unknown, Response = unknown > { - /** - * The number of worker choice strategies execution retries. - */ - public retriesCount: number - /** * The default worker choice strategy in the context. */ @@ -90,7 +85,6 @@ export class WorkerChoiceStrategiesContext< buildWorkerChoiceStrategiesTaskStatisticsRequirements( this.workerChoiceStrategies ) - this.retriesCount = 0 this.retries = getWorkerChoiceStrategiesRetries( this.pool, opts @@ -121,6 +115,17 @@ export class WorkerChoiceStrategiesContext< return this.workerChoiceStrategiesPolicy } + /** + * Gets the number of worker choice strategies execution retries. + * @returns The number of retries. + */ + public getStrategyRetries (): number { + return Array.from( + this.workerChoiceStrategies, + ([_, workerChoiceStrategy]) => workerChoiceStrategy.retriesCount + ).reduce((accumulator, retries) => accumulator + retries, 0) + } + /** * Gets the active worker choice strategies in the context task statistics requirements. * @returns The strategies task statistics requirements. @@ -242,13 +247,13 @@ export class WorkerChoiceStrategiesContext< let workerNodeKey: number | undefined = workerChoiceStrategy.choose() let retriesCount = 0 while (workerNodeKey == null && retriesCount < this.retries) { - workerNodeKey = workerChoiceStrategy.choose() retriesCount++ - this.retriesCount++ + workerNodeKey = workerChoiceStrategy.choose() } + workerChoiceStrategy.retriesCount = retriesCount if (workerNodeKey == null) { throw new Error( - `Worker node key chosen by ${workerChoiceStrategy.name} is null or undefined after ${retriesCount.toString()} retries (max: ${this.retries.toString()})` + `Worker node key chosen by ${workerChoiceStrategy.name} is null or undefined after ${workerChoiceStrategy.retriesCount.toString()} retries (max: ${this.retries.toString()})` ) } return workerNodeKey diff --git a/src/worker/abstract-worker.ts b/src/worker/abstract-worker.ts index 0741963c5..ba8b00aa0 100644 --- a/src/worker/abstract-worker.ts +++ b/src/worker/abstract-worker.ts @@ -198,14 +198,10 @@ export abstract class AbstractWorker< DEFAULT_TASK_NAME, this.taskFunctions.get(DEFAULT_TASK_NAME) ), - ...(defaultTaskFunctionName !== DEFAULT_TASK_NAME - ? [ - buildTaskFunctionProperties( - defaultTaskFunctionName, - this.taskFunctions.get(defaultTaskFunctionName) - ), - ] - : []), + buildTaskFunctionProperties( + defaultTaskFunctionName, + this.taskFunctions.get(defaultTaskFunctionName) + ), ...taskFunctionsProperties, ] } diff --git a/tests/pools/selection-strategies/selection-strategies.test.mjs b/tests/pools/selection-strategies/selection-strategies.test.mjs index 276579280..c188949d3 100644 --- a/tests/pools/selection-strategies/selection-strategies.test.mjs +++ b/tests/pools/selection-strategies/selection-strategies.test.mjs @@ -100,6 +100,11 @@ describe('Selection strategies test suite', () => { workerChoiceStrategy ).name ).toBe(workerChoiceStrategy) + expect( + pool.workerChoiceStrategiesContext.workerChoiceStrategies.get( + workerChoiceStrategy + ).retriesCount + ).toBe(0) expect( pool.workerChoiceStrategiesContext.workerChoiceStrategies.get( workerChoiceStrategy diff --git a/tests/pools/selection-strategies/worker-choice-strategies-context.test.mjs b/tests/pools/selection-strategies/worker-choice-strategies-context.test.mjs index 9bb0ea793..f3e7bf428 100644 --- a/tests/pools/selection-strategies/worker-choice-strategies-context.test.mjs +++ b/tests/pools/selection-strategies/worker-choice-strategies-context.test.mjs @@ -1,5 +1,5 @@ import { expect } from '@std/expect' -import { createStubInstance, restore, stub } from 'sinon' +import { restore, stub } from 'sinon' import { DynamicThreadPool, @@ -84,72 +84,83 @@ describe('Worker choice strategies context test suite', () => { expect(workerChoiceStrategiesContext.defaultWorkerChoiceStrategy).toBe( WorkerChoiceStrategies.ROUND_ROBIN ) - const workerChoiceStrategyUndefinedStub = createStubInstance( - RoundRobinWorkerChoiceStrategy, - { - choose: stub().returns(undefined), - } - ) - workerChoiceStrategiesContext.workerChoiceStrategies.set( - workerChoiceStrategiesContext.defaultWorkerChoiceStrategy, - workerChoiceStrategyUndefinedStub - ) - expect(() => workerChoiceStrategiesContext.execute()).toThrow( - new Error( - `Worker node key chosen by ${workerChoiceStrategyUndefinedStub.name} is null or undefined after ${workerChoiceStrategiesContext.retries.toString()} retries (max: ${workerChoiceStrategiesContext.retries.toString()})` + const workerChoiceStrategyUndefinedStub = + workerChoiceStrategiesContext.workerChoiceStrategies.get( + workerChoiceStrategiesContext.defaultWorkerChoiceStrategy ) - ) - const workerChoiceStrategyNullStub = createStubInstance( - RoundRobinWorkerChoiceStrategy, - { - choose: stub().returns(null), - } - ) - workerChoiceStrategiesContext.workerChoiceStrategies.set( - workerChoiceStrategiesContext.defaultWorkerChoiceStrategy, - workerChoiceStrategyNullStub - ) - expect(() => workerChoiceStrategiesContext.execute()).toThrow( - new Error( - `Worker node key chosen by ${workerChoiceStrategyNullStub.name} is null or undefined after ${workerChoiceStrategiesContext.retries.toString()} retries (max: ${workerChoiceStrategiesContext.retries.toString()})` + const chooseUndefinedStub = stub( + workerChoiceStrategyUndefinedStub, + 'choose' + ).returns(undefined) + let err + try { + workerChoiceStrategiesContext.execute() + } catch (e) { + err = e + } + expect(err).toBeInstanceOf(Error) + expect(err.message).toBe( + `Worker node key chosen by ${workerChoiceStrategyUndefinedStub.name} is null or undefined after ${workerChoiceStrategyUndefinedStub.retriesCount.toString()} retries (max: ${workerChoiceStrategiesContext.retries.toString()})` + ) + expect(chooseUndefinedStub.callCount).toBe( + workerChoiceStrategyUndefinedStub.retriesCount + 1 + ) + expect(workerChoiceStrategiesContext.getStrategyRetries()).toBe( + workerChoiceStrategyUndefinedStub.retriesCount + ) + chooseUndefinedStub.restore() + const workerChoiceStrategyNullStub = + workerChoiceStrategiesContext.workerChoiceStrategies.get( + workerChoiceStrategiesContext.defaultWorkerChoiceStrategy ) - ) + const chooseNullStub = stub(workerChoiceStrategyNullStub, 'choose').returns( + null + ) + err = undefined + try { + workerChoiceStrategiesContext.execute() + } catch (e) { + err = e + } + expect(err).toBeInstanceOf(Error) + expect(err.message).toBe( + `Worker node key chosen by ${workerChoiceStrategyNullStub.name} is null or undefined after ${workerChoiceStrategyNullStub.retriesCount.toString()} retries (max: ${workerChoiceStrategiesContext.retries.toString()})` + ) + expect(chooseNullStub.callCount).toBe( + workerChoiceStrategyNullStub.retriesCount + 1 + ) + expect(workerChoiceStrategiesContext.getStrategyRetries()).toBe( + workerChoiceStrategyNullStub.retriesCount + ) + chooseNullStub.restore() }) it('Verify that execute() retry until a worker node is chosen', () => { const workerChoiceStrategiesContext = new WorkerChoiceStrategiesContext( fixedPool ) - const workerChoiceStrategyStub = createStubInstance( - RoundRobinWorkerChoiceStrategy, - { - choose: stub() - .onCall(0) - .returns(undefined) - .onCall(1) - .returns(undefined) - .onCall(2) - .returns(undefined) - .onCall(3) - .returns(undefined) - .onCall(4) - .returns(undefined) - .returns(1), - } - ) expect(workerChoiceStrategiesContext.defaultWorkerChoiceStrategy).toBe( WorkerChoiceStrategies.ROUND_ROBIN ) - workerChoiceStrategiesContext.workerChoiceStrategies.set( - workerChoiceStrategiesContext.defaultWorkerChoiceStrategy, - workerChoiceStrategyStub - ) - const chosenWorkerKey = workerChoiceStrategiesContext.execute() - expect( + const workerChoiceStrategyStub = workerChoiceStrategiesContext.workerChoiceStrategies.get( workerChoiceStrategiesContext.defaultWorkerChoiceStrategy - ).choose.callCount - ).toBe(6) + ) + stub(workerChoiceStrategyStub, 'choose') + .onCall(0) + .returns(undefined) + .onCall(1) + .returns(undefined) + .onCall(2) + .returns(undefined) + .onCall(3) + .returns(undefined) + .onCall(4) + .returns(undefined) + .returns(1) + const chosenWorkerKey = workerChoiceStrategiesContext.execute() + expect(workerChoiceStrategyStub.choose.callCount).toBe(6) + expect(workerChoiceStrategiesContext.getStrategyRetries()).toBe(5) expect(chosenWorkerKey).toBe(1) }) @@ -157,25 +168,17 @@ describe('Worker choice strategies context test suite', () => { const workerChoiceStrategiesContext = new WorkerChoiceStrategiesContext( fixedPool ) - const workerChoiceStrategyStub = createStubInstance( - RoundRobinWorkerChoiceStrategy, - { - choose: stub().returns(0), - } - ) expect(workerChoiceStrategiesContext.defaultWorkerChoiceStrategy).toBe( WorkerChoiceStrategies.ROUND_ROBIN ) - workerChoiceStrategiesContext.workerChoiceStrategies.set( - workerChoiceStrategiesContext.defaultWorkerChoiceStrategy, - workerChoiceStrategyStub - ) - const chosenWorkerKey = workerChoiceStrategiesContext.execute() - expect( + const workerChoiceStrategyStub = workerChoiceStrategiesContext.workerChoiceStrategies.get( workerChoiceStrategiesContext.defaultWorkerChoiceStrategy - ).choose.calledOnce - ).toBe(true) + ) + stub(workerChoiceStrategyStub, 'choose').returns(0) + const chosenWorkerKey = workerChoiceStrategiesContext.execute() + expect(workerChoiceStrategyStub.choose.calledOnce).toBe(true) + expect(workerChoiceStrategiesContext.getStrategyRetries()).toBe(0) expect(chosenWorkerKey).toBe(0) }) @@ -183,25 +186,17 @@ describe('Worker choice strategies context test suite', () => { const workerChoiceStrategiesContext = new WorkerChoiceStrategiesContext( dynamicPool ) - const workerChoiceStrategyStub = createStubInstance( - RoundRobinWorkerChoiceStrategy, - { - choose: stub().returns(0), - } - ) expect(workerChoiceStrategiesContext.defaultWorkerChoiceStrategy).toBe( WorkerChoiceStrategies.ROUND_ROBIN ) - workerChoiceStrategiesContext.workerChoiceStrategies.set( - workerChoiceStrategiesContext.defaultWorkerChoiceStrategy, - workerChoiceStrategyStub - ) - const chosenWorkerKey = workerChoiceStrategiesContext.execute() - expect( + const workerChoiceStrategyStub = workerChoiceStrategiesContext.workerChoiceStrategies.get( workerChoiceStrategiesContext.defaultWorkerChoiceStrategy - ).choose.calledOnce - ).toBe(true) + ) + stub(workerChoiceStrategyStub, 'choose').returns(0) + const chosenWorkerKey = workerChoiceStrategiesContext.execute() + expect(workerChoiceStrategyStub.choose.calledOnce).toBe(true) + expect(workerChoiceStrategiesContext.getStrategyRetries()).toBe(0) expect(chosenWorkerKey).toBe(0) })