From 02ab9aa806db67db794e0b1480d3ad4a11c13bb2 Mon Sep 17 00:00:00 2001 From: Aleksandras Novikovas Date: Fri, 10 Mar 2017 17:46:23 +0200 Subject: [PATCH 1/4] Added option.maxTries Setting this option stops trying to create resource after specified consequent failures. Prevents pool going into infinite loop if resource can not be created. --- README.md | 1 + lib/Pool.js | 53 ++++++++++++++++++++++++++++++++++++++++------ lib/PoolOptions.js | 5 +++++ lib/errors.js | 9 +++++++- 4 files changed, 61 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 4c577d2a..a02a86e4 100644 --- a/README.md +++ b/README.md @@ -137,6 +137,7 @@ An optional object/dictionary with the any of the following properties: - `numTestsPerRun`: Number of resources to check each eviction run. Default: 3. - `softIdleTimeoutMillis`: amount of time an object may sit idle in the pool before it is eligible for eviction by the idle object evictor (if any), with the extra condition that at least "min idle" object instances remain in the pool. Default -1 (nothing can get evicted) - `idleTimeoutMillis`: the minimum amount of time that an object may sit idle in the pool before it is eligible for eviction due to idle time. Supercedes `softIdleTimeoutMillis` Default: 30000 +- `maxTries`: maximum number of consequent failures before pool stops trying to create resource and rejects resource request. Default: 0 (tries forever until resource is created). - `Promise`: Promise lib, a Promises/A+ implementation that the pool should use. Defaults to whatever `global.Promise` is (usually native promises). ### pool.acquire diff --git a/lib/Pool.js b/lib/Pool.js index 70bfd5a3..92c90515 100644 --- a/lib/Pool.js +++ b/lib/Pool.js @@ -3,6 +3,7 @@ const EventEmitter = require("events").EventEmitter; const factoryValidator = require("./factoryValidator"); +const errors = require('./errors') const PoolOptions = require("./PoolOptions"); const ResourceRequest = require("./ResourceRequest"); const ResourceLoan = require("./ResourceLoan"); @@ -123,6 +124,12 @@ class Pool extends EventEmitter { */ this._scheduledEviction = null; + /** + * counter of consequent failures to create resource + * @type {Number} + */ + this._failedTries = 0 + // create initial resources (if factory.min > 0) if (this._config.autostart === true) { this.start(); @@ -215,6 +222,15 @@ class Pool extends EventEmitter { return; } + // Try counter exceeded - reject resource request to next waiting client + if ( + this._config.maxTries > 0 && + this._failedTries >= this._config.maxTries + ) { + this._dispatchPooledResourceToNextWaitingClient() + return + } + const resourceShortfall = numWaitingClients - this._potentiallyAllocableResourceCount; @@ -267,15 +283,27 @@ class Pool extends EventEmitter { ) { // While we were away either all the waiting clients timed out // or were somehow fulfilled. put our pooledResource back. - this._addPooledResourceToAvailableObjects(pooledResource); + if (pooledResource) { + // While we were away either all the waiting clients timed out + // or were somehow fulfilled. put our pooledResource back. + this._addPooledResourceToAvailableObjects(pooledResource) + } // TODO: do need to trigger anything before we leave? return false; } - const loan = new ResourceLoan(pooledResource, this._Promise); - this._resourceLoans.set(pooledResource.obj, loan); - pooledResource.allocate(); - clientResourceRequest.resolve(pooledResource.obj); - return true; + if (pooledResource) { + const loan = new ResourceLoan(pooledResource, this._Promise) + this._resourceLoans.set(pooledResource.obj, loan) + pooledResource.allocate() + clientResourceRequest.resolve(pooledResource.obj) + } else { + let errorMessage = 'Failed to create resource' + if (this._config.maxTries > 0 && this._failedTries >= this._config.maxTries) { + errorMessage = 'Failed to create resource ' + this._failedTries + ' times in a row' + } + clientResourceRequest.reject(new errors.ResourceCreationError(errorMessage)) + } + return true } /** @@ -304,12 +332,21 @@ class Pool extends EventEmitter { * @private */ _createResource() { + // Do not attempt to create resource if reached maximum number of consequent failures + if ( + this._config.maxTries > 0 && + this._failedTries >= this._config.maxTries + ) { + return + } // An attempt to create a resource const factoryPromise = this._factory.create(); const wrappedFactoryPromise = this._Promise.resolve(factoryPromise); this._trackOperation(wrappedFactoryPromise, this._factoryCreateOperations) .then(resource => { + // Resource created successfully - reset fail counter + this._failedTries = 0 this._handleNewResource(resource); return null; }) @@ -435,6 +472,10 @@ class Pool extends EventEmitter { ); } + // Reset fail counter - maybe resource will be created this time + // (for example network is restored and connection can be made) + this._failedTries = 0 + // TODO: should we defer this check till after this event loop incase "the situation" changes in the meantime if ( this._config.maxWaitingClients !== undefined && diff --git a/lib/PoolOptions.js b/lib/PoolOptions.js index 498c0e34..a4d3c3a9 100644 --- a/lib/PoolOptions.js +++ b/lib/PoolOptions.js @@ -42,6 +42,9 @@ class PoolOptions { * @param {Number} [opts.idleTimeoutMillis=30000] * the minimum amount of time that an object may sit idle in the pool before it is eligible for eviction * due to idle time. Supercedes "softIdleTimeoutMillis" Default: 30000 + * @param {Number} opts.maxTries + * maximum number of consequent failures before pool stops trying to create resource + * and rejects resource request. Default: 0 (tries forever until resource is created). * @param {typeof Promise} [opts.Promise=Promise] * What promise implementation should the pool use, defaults to native promises. */ @@ -81,9 +84,11 @@ class PoolOptions { this.max = parseInt(opts.max, 10); // @ts-ignore this.min = parseInt(opts.min, 10); + this.maxTries = parseInt(opts.maxTries, 10) this.max = Math.max(isNaN(this.max) ? 1 : this.max, 1); this.min = Math.min(isNaN(this.min) ? 0 : this.min, this.max); + this.maxTries = (isNaN(this.maxTries) ? 0 : this.maxTries) this.evictionRunIntervalMillis = opts.evictionRunIntervalMillis || poolDefaults.evictionRunIntervalMillis; diff --git a/lib/errors.js b/lib/errors.js index b02d822f..bdeb881f 100644 --- a/lib/errors.js +++ b/lib/errors.js @@ -20,8 +20,15 @@ class TimeoutError extends ExtendableError { super(m); } } + +class ResourceCreationError extends ExtendableError { + constructor (m) { + super(m) + } +} /* eslint-enable no-useless-constructor */ module.exports = { - TimeoutError: TimeoutError + TimeoutError: TimeoutError, + ResourceCreationError: ResourceCreationError }; From bc90c45e44a9fa5f65cf37fb8e2f04b5879973c3 Mon Sep 17 00:00:00 2001 From: Aleksandras Novikovas Date: Sun, 11 Feb 2018 16:04:36 +0200 Subject: [PATCH 2/4] Fixed syntax to pass tests. --- lib/Pool.js | 37 +++++++++++++++++++++---------------- lib/PoolOptions.js | 4 ++-- lib/errors.js | 4 ++-- 3 files changed, 25 insertions(+), 20 deletions(-) diff --git a/lib/Pool.js b/lib/Pool.js index 92c90515..bfbc3e26 100644 --- a/lib/Pool.js +++ b/lib/Pool.js @@ -3,7 +3,7 @@ const EventEmitter = require("events").EventEmitter; const factoryValidator = require("./factoryValidator"); -const errors = require('./errors') +const errors = require("./errors"); const PoolOptions = require("./PoolOptions"); const ResourceRequest = require("./ResourceRequest"); const ResourceLoan = require("./ResourceLoan"); @@ -128,7 +128,7 @@ class Pool extends EventEmitter { * counter of consequent failures to create resource * @type {Number} */ - this._failedTries = 0 + this._failedTries = 0; // create initial resources (if factory.min > 0) if (this._config.autostart === true) { @@ -227,8 +227,8 @@ class Pool extends EventEmitter { this._config.maxTries > 0 && this._failedTries >= this._config.maxTries ) { - this._dispatchPooledResourceToNextWaitingClient() - return + this._dispatchPooledResourceToNextWaitingClient(); + return; } const resourceShortfall = @@ -286,24 +286,29 @@ class Pool extends EventEmitter { if (pooledResource) { // While we were away either all the waiting clients timed out // or were somehow fulfilled. put our pooledResource back. - this._addPooledResourceToAvailableObjects(pooledResource) + this._addPooledResourceToAvailableObjects(pooledResource); } // TODO: do need to trigger anything before we leave? return false; } if (pooledResource) { - const loan = new ResourceLoan(pooledResource, this._Promise) - this._resourceLoans.set(pooledResource.obj, loan) - pooledResource.allocate() - clientResourceRequest.resolve(pooledResource.obj) + const loan = new ResourceLoan(pooledResource, this._Promise); + this._resourceLoans.set(pooledResource.obj, loan); + pooledResource.allocate(); + clientResourceRequest.resolve(pooledResource.obj); } else { - let errorMessage = 'Failed to create resource' - if (this._config.maxTries > 0 && this._failedTries >= this._config.maxTries) { - errorMessage = 'Failed to create resource ' + this._failedTries + ' times in a row' + let errorMessage = "Failed to create resource"; + if ( + this._config.maxTries > 0 && + this._failedTries >= this._config.maxTries + ) { + errorMessage = "Failed to create resource " + this._failedTries + " times in a row" } - clientResourceRequest.reject(new errors.ResourceCreationError(errorMessage)) + clientResourceRequest.reject( + new errors.ResourceCreationError(errorMessage) + ); } - return true + return true; } /** @@ -337,7 +342,7 @@ class Pool extends EventEmitter { this._config.maxTries > 0 && this._failedTries >= this._config.maxTries ) { - return + return; } // An attempt to create a resource const factoryPromise = this._factory.create(); @@ -474,7 +479,7 @@ class Pool extends EventEmitter { // Reset fail counter - maybe resource will be created this time // (for example network is restored and connection can be made) - this._failedTries = 0 + this._failedTries = 0; // TODO: should we defer this check till after this event loop incase "the situation" changes in the meantime if ( diff --git a/lib/PoolOptions.js b/lib/PoolOptions.js index a4d3c3a9..c4f8a88d 100644 --- a/lib/PoolOptions.js +++ b/lib/PoolOptions.js @@ -84,11 +84,11 @@ class PoolOptions { this.max = parseInt(opts.max, 10); // @ts-ignore this.min = parseInt(opts.min, 10); - this.maxTries = parseInt(opts.maxTries, 10) + this.maxTries = parseInt(opts.maxTries, 10); this.max = Math.max(isNaN(this.max) ? 1 : this.max, 1); this.min = Math.min(isNaN(this.min) ? 0 : this.min, this.max); - this.maxTries = (isNaN(this.maxTries) ? 0 : this.maxTries) + this.maxTries = isNaN(this.maxTries) ? 0 : this.maxTries; this.evictionRunIntervalMillis = opts.evictionRunIntervalMillis || poolDefaults.evictionRunIntervalMillis; diff --git a/lib/errors.js b/lib/errors.js index bdeb881f..31650a1f 100644 --- a/lib/errors.js +++ b/lib/errors.js @@ -22,8 +22,8 @@ class TimeoutError extends ExtendableError { } class ResourceCreationError extends ExtendableError { - constructor (m) { - super(m) + constructor(m) { + super(m); } } /* eslint-enable no-useless-constructor */ From a38b15b43d4461addb5e137f3480a7d487ef966a Mon Sep 17 00:00:00 2001 From: Aleksandras Novikovas Date: Sun, 11 Feb 2018 16:12:12 +0200 Subject: [PATCH 3/4] Fix syntax to pass tests (try 2) --- lib/Pool.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/Pool.js b/lib/Pool.js index bfbc3e26..bc411455 100644 --- a/lib/Pool.js +++ b/lib/Pool.js @@ -302,7 +302,8 @@ class Pool extends EventEmitter { this._config.maxTries > 0 && this._failedTries >= this._config.maxTries ) { - errorMessage = "Failed to create resource " + this._failedTries + " times in a row" + errorMessage = + "Failed to create resource " + this._failedTries + " times in a row"; } clientResourceRequest.reject( new errors.ResourceCreationError(errorMessage) @@ -351,7 +352,7 @@ class Pool extends EventEmitter { this._trackOperation(wrappedFactoryPromise, this._factoryCreateOperations) .then(resource => { // Resource created successfully - reset fail counter - this._failedTries = 0 + this._failedTries = 0; this._handleNewResource(resource); return null; }) From 5559c289063b7eb4788d97990bca7ca247f11348 Mon Sep 17 00:00:00 2001 From: Aleksandras-Novikovas Date: Sun, 13 May 2018 20:06:03 +0300 Subject: [PATCH 4/4] Readded _failedTries incrementatiion (disapeared in last commit). Added autotest for maxTries. --- lib/Pool.js | 1 + test/generic-pool-maxtries-test.js | 76 ++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) create mode 100644 test/generic-pool-maxtries-test.js diff --git a/lib/Pool.js b/lib/Pool.js index bc411455..d1b59441 100644 --- a/lib/Pool.js +++ b/lib/Pool.js @@ -357,6 +357,7 @@ class Pool extends EventEmitter { return null; }) .catch(reason => { + this._failedTries++; this.emit(FACTORY_CREATE_ERROR, reason); this._dispense(); }); diff --git a/test/generic-pool-maxtries-test.js b/test/generic-pool-maxtries-test.js new file mode 100644 index 00000000..e31ebcaa --- /dev/null +++ b/test/generic-pool-maxtries-test.js @@ -0,0 +1,76 @@ +"use strict"; + +const tap = require("tap"); +const createPool = require("../").createPool; + +tap.test("maxTries handles acquire exhausted calls", function(t) { + let resourceCreationAttempts = 0; + const factory = { + create: function() { + resourceCreationAttempts++; + if (resourceCreationAttempts < 5) { + return Promise.reject(new Error("Create Error")); + } + return Promise.resolve({}); + }, + destroy: function() { + return Promise.resolve(); + } + }; + const config = { + maxTries: 3 + }; + + const pool = createPool(factory, config); + + pool + .acquire() + .then(function(resource) { + t.fail("wooops"); + }) + .catch(function(err) { + t.match(err, /ResourceCreationError: Failed to create resource/); + return pool.drain(); + }) + .then(function() { + return pool.clear(); + }) + .then(function() {}) + .then(t.end) + .catch(t.error); +}); + +tap.test("maxTries handles acquire non exhausted calls", function(t) { + const myResource = {}; + let resourceCreationAttempts = 0; + const factory = { + create: function() { + resourceCreationAttempts++; + if (resourceCreationAttempts < 2) { + return Promise.reject(new Error("Create Error")); + } + return Promise.resolve(myResource); + }, + destroy: function() { + return Promise.resolve(); + } + }; + const config = { + maxTries: 3 + }; + + const pool = createPool(factory, config); + + pool + .acquire() + .then(function(resource) { + t.equal(resource, myResource); + pool.release(resource); + return pool.drain(); + }) + .then(function() { + return pool.clear(); + }) + .then(t.end) + .catch(t.error); +});