From 8d3782faec08afef3370aaaadd06de2c1cb270fd Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=B4me=20Benoit?= Date: Mon, 22 Feb 2021 19:02:10 +0100 Subject: [PATCH] Check that a pool have a minimum number of workers (#213) MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit * Check that a pool have at least one worker, Signed-off-by: Jérôme Benoit * Specify return type on checkNumberOfWorkers Signed-off-by: Jérôme Benoit * Fix dynamic pool handling and add unit tests. Signed-off-by: Jérôme Benoit * Add more sanity checks to numberOfWorkers. Signed-off-by: Jérôme Benoit * Increase branches coverage. Signed-off-by: Jérôme Benoit * Renable skipped test on cluster worker. Signed-off-by: Jérôme Benoit * Remove wrong comment. Signed-off-by: Jérôme Benoit * Try to increase coverage by alternating fixed pool type. Signed-off-by: Jérôme Benoit * Address review comments. Signed-off-by: Jérôme Benoit * Sync package-lock.json Signed-off-by: Jérôme Benoit * Remove debug log. Signed-off-by: Jérôme Benoit * Fix tests (#218) * Fix the unit test for the number of workers. Signed-off-by: Jérôme Benoit * Cleanups. Signed-off-by: Jérôme Benoit Co-authored-by: Shinigami --- benchmarks/README.md | 2 +- .../functions/function-to-bench.js | 2 +- package-lock.json | 2 +- src/pools/abstract-pool.ts | 19 ++++++ tests/pools/abstract/abstract-pool.test.js | 65 ++++++++++++++----- tests/pools/cluster/dynamic.test.js | 17 +++++ tests/pools/cluster/fixed.test.js | 7 ++ tests/pools/selection-strategies.test.js | 3 +- tests/pools/thread/dynamic.test.js | 17 +++-- tests/pools/thread/fixed.test.js | 6 ++ tests/worker/abstract-worker.test.js | 8 +-- tests/worker/cluster-worker.test.js | 3 +- tests/worker/thread-worker.test.js | 3 +- 13 files changed, 119 insertions(+), 35 deletions(-) diff --git a/benchmarks/README.md b/benchmarks/README.md index 5017588e..c488a23e 100644 --- a/benchmarks/README.md +++ b/benchmarks/README.md @@ -19,7 +19,7 @@ External pools with which we compared the poolifier results: Those are our results: - CPU Intensive task with 100k operations submitted to each pool [BENCH-100000.MD](./versus-external-pools/BENCH-100000.MD). -This benchmark ran on a MacBook Pro 2015, 2,2 GHz Intel Core i7 quad-core, 16 GB 1600 MHz DDR3. + This benchmark ran on a MacBook Pro 2015, 2,2 GHz Intel Core i7 quad-core, 16 GB 1600 MHz DDR3. > :warning: **We would need funds to run our benchmarks more often and on Cloud VMs, please consider to sponsor this project** diff --git a/benchmarks/versus-external-pools/functions/function-to-bench.js b/benchmarks/versus-external-pools/functions/function-to-bench.js index 0c9738e6..23312bfc 100644 --- a/benchmarks/versus-external-pools/functions/function-to-bench.js +++ b/benchmarks/versus-external-pools/functions/function-to-bench.js @@ -1,5 +1,5 @@ module.exports = function (data) { - if ( data.taskType === 'CPU_INTENSIVE' ) { + if (data.taskType === 'CPU_INTENSIVE') { // CPU Intensive task for (let i = 0; i <= 5000; i++) { const o = { diff --git a/package-lock.json b/package-lock.json index 2729f787..76342153 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "poolifier", - "version": "v2.0.0-beta.6", + "version": "2.0.0-beta.7", "lockfileVersion": 1, "requires": true, "dependencies": { diff --git a/src/pools/abstract-pool.ts b/src/pools/abstract-pool.ts index c123849e..f1ac76ed 100644 --- a/src/pools/abstract-pool.ts +++ b/src/pools/abstract-pool.ts @@ -162,6 +162,7 @@ export abstract class AbstractPool< if (!this.isMain()) { throw new Error('Cannot start a pool from a worker!') } + this.checkNumberOfWorkers(this.numberOfWorkers) this.checkFilePath(this.filePath) this.setupHook() @@ -182,6 +183,24 @@ export abstract class AbstractPool< } } + private checkNumberOfWorkers (numberOfWorkers: number): void { + if (numberOfWorkers == null) { + throw new Error( + 'Cannot instantiate a pool without specifying the number of workers' + ) + } else if (!Number.isSafeInteger(numberOfWorkers)) { + throw new Error( + 'Cannot instantiate a pool with a non integer number of workers' + ) + } else if (numberOfWorkers < 0) { + throw new Error( + 'Cannot instantiate a pool with a negative number of workers' + ) + } else if (!this.isDynamic() && numberOfWorkers === 0) { + throw new Error('Cannot instantiate a fixed pool with no worker') + } + } + /** @inheritdoc */ public isDynamic (): boolean { return false diff --git a/tests/pools/abstract/abstract-pool.test.js b/tests/pools/abstract/abstract-pool.test.js index 9b32a7e0..482ce5e3 100644 --- a/tests/pools/abstract/abstract-pool.test.js +++ b/tests/pools/abstract/abstract-pool.test.js @@ -1,5 +1,5 @@ const expect = require('expect') -const { FixedThreadPool } = require('../../../lib/index') +const { FixedClusterPool, FixedThreadPool } = require('../../../lib/index') const expectedError = new Error('Worker could not be found in tasks map') class StubPoolWithTasksMapClear extends FixedThreadPool { @@ -18,7 +18,7 @@ describe('Abstract pool test suite', () => { it('Simulate worker not found during increaseWorkersTask', () => { const pool = new StubPoolWithTasksMapClear( 1, - './tests/worker-files/cluster/testWorker.js', + './tests/worker-files/thread/testWorker.js', { errorHandler: e => console.error(e) } @@ -31,7 +31,7 @@ describe('Abstract pool test suite', () => { it('Simulate worker not found during decreaseWorkersTasks', () => { const pool = new StubPoolWithTasksMapClear( 1, - './tests/worker-files/cluster/testWorker.js', + './tests/worker-files/thread/testWorker.js', { errorHandler: e => console.error(e) } @@ -42,23 +42,52 @@ describe('Abstract pool test suite', () => { }) it('Simulate pool creation from a non main thread/process', () => { - expect(() => { - const pool = new StubPoolWithIsMainMethod( - 1, - './tests/worker-files/cluster/testWorker.js', - { - errorHandler: e => console.error(e) - } - ) - }).toThrowError() + expect( + () => + new StubPoolWithIsMainMethod( + 1, + './tests/worker-files/thread/testWorker.js', + { + errorHandler: e => console.error(e) + } + ) + ).toThrowError(new Error('Cannot start a pool from a worker!')) }) it('Verify that filePath is checked', () => { - expect(() => { - const pool = new StubPoolWithIsMainMethod(1).toThrowError() - }) - expect(() => { - const pool = new StubPoolWithIsMainMethod(1, '').toThrowError() - }) + expect(() => new StubPoolWithIsMainMethod(1)).toThrowError( + new Error('Cannot start a pool from a worker!') + ) + expect(() => new StubPoolWithIsMainMethod(1, '')).toThrowError( + new Error('Cannot start a pool from a worker!') + ) + }) + + it('Verify that numberOfWorkers is checked', () => { + expect(() => new FixedThreadPool()).toThrowError( + new Error( + 'Cannot instantiate a pool without specifying the number of workers' + ) + ) + }) + + it('Verify that a negative number of workers is checked', () => { + expect( + () => + new FixedClusterPool(-1, './tests/worker-files/cluster/testWorker.js') + ).toThrowError( + new Error('Cannot instantiate a pool with a negative number of workers') + ) + }) + + it('Verify that a non integer number of workers is checked', () => { + expect( + () => + new FixedThreadPool(0.25, './tests/worker-files/thread/testWorker.js') + ).toThrowError( + new Error( + 'Cannot instantiate a pool with a non integer number of workers' + ) + ) }) }) diff --git a/tests/pools/cluster/dynamic.test.js b/tests/pools/cluster/dynamic.test.js index 53a5f8d0..05d8cdd4 100644 --- a/tests/pools/cluster/dynamic.test.js +++ b/tests/pools/cluster/dynamic.test.js @@ -56,6 +56,12 @@ describe('Dynamic cluster pool test suite', () => { expect(res).toBe(min) }) + it('Validation of inputs test', () => { + expect(() => new DynamicClusterPool(min)).toThrowError( + new Error('Please specify a file with a worker implementation') + ) + }) + it('Should work even without opts in input', async () => { const pool1 = new DynamicClusterPool( 1, @@ -103,4 +109,15 @@ describe('Dynamic cluster pool test suite', () => { // We need to clean up the resources after our test await longRunningPool.destroy() }) + + it('Verify that a pool with zero worker can be instantiated', async () => { + const pool = new DynamicClusterPool( + 0, + max, + './tests/worker-files/cluster/testWorker.js' + ) + expect(pool).toBeInstanceOf(DynamicClusterPool) + // We need to clean up the resources after our test + await pool.destroy() + }) }) diff --git a/tests/pools/cluster/fixed.test.js b/tests/pools/cluster/fixed.test.js index 1137b407..0eb21d07 100644 --- a/tests/pools/cluster/fixed.test.js +++ b/tests/pools/cluster/fixed.test.js @@ -140,4 +140,11 @@ describe('Fixed cluster pool test suite', () => { // We need to clean up the resources after our test await pool1.destroy() }) + + it('Verify that a pool with zero worker fails', async () => { + expect( + () => + new FixedClusterPool(0, './tests/worker-files/cluster/testWorker.js') + ).toThrowError(new Error('Cannot instantiate a fixed pool with no worker')) + }) }) diff --git a/tests/pools/selection-strategies.test.js b/tests/pools/selection-strategies.test.js index 186dee7b..01636b67 100644 --- a/tests/pools/selection-strategies.test.js +++ b/tests/pools/selection-strategies.test.js @@ -4,7 +4,6 @@ const { DynamicThreadPool, FixedThreadPool } = require('../../lib/index') -const TestUtils = require('../test-utils') describe('Selection strategies test suite', () => { it('Verify that WorkerChoiceStrategies enumeration provides string values', () => { @@ -12,7 +11,7 @@ describe('Selection strategies test suite', () => { expect(WorkerChoiceStrategies.LESS_RECENTLY_USED).toBe('LESS_RECENTLY_USED') }) - it('Verify LESS_RECENTLY_USED is taken', async () => { + it('Verify LESS_RECENTLY_USED strategy is taken', async () => { const max = 3 const pool = new FixedThreadPool( max, diff --git a/tests/pools/thread/dynamic.test.js b/tests/pools/thread/dynamic.test.js index a7f53734..4ee24720 100644 --- a/tests/pools/thread/dynamic.test.js +++ b/tests/pools/thread/dynamic.test.js @@ -59,10 +59,8 @@ describe('Dynamic thread pool test suite', () => { expect(closedThreads).toBe(min) }) - it('Validations test', () => { - expect(() => { - const pool1 = new DynamicThreadPool() - }).toThrowError( + it('Validation of inputs test', () => { + expect(() => new DynamicThreadPool(min)).toThrowError( new Error('Please specify a file with a worker implementation') ) }) @@ -121,4 +119,15 @@ describe('Dynamic thread pool test suite', () => { // We need to clean up the resources after our test await longRunningPool.destroy() }) + + it('Verify that a pool with zero worker can be instantiated', async () => { + const pool = new DynamicThreadPool( + 0, + max, + './tests/worker-files/thread/testWorker.js' + ) + expect(pool).toBeInstanceOf(DynamicThreadPool) + // We need to clean up the resources after our test + await pool.destroy() + }) }) diff --git a/tests/pools/thread/fixed.test.js b/tests/pools/thread/fixed.test.js index cea55025..9a2d74d3 100644 --- a/tests/pools/thread/fixed.test.js +++ b/tests/pools/thread/fixed.test.js @@ -118,4 +118,10 @@ describe('Fixed thread pool test suite', () => { // We need to clean up the resources after our test await pool1.destroy() }) + + it('Verify that a pool with zero worker fails', async () => { + expect( + () => new FixedThreadPool(0, './tests/worker-files/thread/testWorker.js') + ).toThrowError(new Error('Cannot instantiate a fixed pool with no worker')) + }) }) diff --git a/tests/worker/abstract-worker.test.js b/tests/worker/abstract-worker.test.js index ddb8427b..0c9b665e 100644 --- a/tests/worker/abstract-worker.test.js +++ b/tests/worker/abstract-worker.test.js @@ -1,10 +1,10 @@ const expect = require('expect') -const { ThreadWorker } = require('../../lib') +const { ClusterWorker } = require('../../lib') describe('Abstract worker test suite', () => { it('Verify that fn function is mandatory', () => { - expect(() => { - const worker = new ThreadWorker() - }).toThrowError() + expect(() => new ClusterWorker()).toThrowError( + new Error('fn parameter is mandatory') + ) }) }) diff --git a/tests/worker/cluster-worker.test.js b/tests/worker/cluster-worker.test.js index fc2d3442..5dec6076 100644 --- a/tests/worker/cluster-worker.test.js +++ b/tests/worker/cluster-worker.test.js @@ -2,8 +2,7 @@ const expect = require('expect') const { ClusterWorker } = require('../../lib') describe('Cluster worker test suite', () => { - // Skipped because ClusterWorker would be in main instead of non-main worker - it.skip('Verify worker has default maxInactiveTime', () => { + it('Verify worker has default maxInactiveTime', () => { const worker = new ClusterWorker(() => {}) expect(worker.maxInactiveTime).toEqual(60_000) }) diff --git a/tests/worker/thread-worker.test.js b/tests/worker/thread-worker.test.js index dfe49655..fd136a73 100644 --- a/tests/worker/thread-worker.test.js +++ b/tests/worker/thread-worker.test.js @@ -2,9 +2,8 @@ const expect = require('expect') const { ThreadWorker } = require('../../lib') let numberOfMessagesPosted = 0 -const postMessage = function (message) { +const postMessage = function () { numberOfMessagesPosted++ - console.log(message) } class SpyWorker extends ThreadWorker { getMainWorker () { -- 2.34.1