From a97149ef96b7ad9a808feddba04e007bfe6c12ff Mon Sep 17 00:00:00 2001 From: Kevin Burke Date: Tue, 16 Feb 2016 01:12:38 -0800 Subject: [PATCH 1/6] Add option to error if the pool is full Currently, if you attempt to acquire a resource and the pool is full, the callback will not run until a space becomes available. This can lead to a deadlock if a pool member's release is blocked on the acquire callback running. To mitigate the deadlock possibility, support a third argument `pool.acquire(cb, priority, {block: false})`, which will immediately hit the callback with an error if the pool is full. Adds a test that this behaves as expected. Adds a helper function pool._isFull, which checks both the _count and the list of _availableObjects - I think we have to check both to ensure that the pool is full, and that this abstraction doesn't exist yet. Callers may want to wait for a small amount of time before giving up on the pool; I'm going to add that option in a second commit. This work was sponsored by [Shyp](https://shyp.com). --- lib/generic-pool.js | 22 +++++++++++---- test/generic-pool.test.js | 56 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 5 deletions(-) diff --git a/lib/generic-pool.js b/lib/generic-pool.js index eb9460c2..a3939271 100644 --- a/lib/generic-pool.js +++ b/lib/generic-pool.js @@ -365,25 +365,37 @@ Pool.prototype._ensureMinimum = function _ensureMinimum () { } } +Pool.prototype._isFull = function() { + return this._count >= this._factory.max && this._availableObjects.length === 0 +} + /** * Request a new client. The callback will be called, - * when a new client will be availabe, passing the client to it. + * when a new client will be available, passing the client to it. * * @param {Function} callback * Callback function to be called after the acquire is successful. * The function will receive the acquired item as the first parameter. * * @param {Number} priority - * Optional. Integer between 0 and (priorityRange - 1). Specifies the priority - * of the caller if there are no available resources. Lower numbers mean higher - * priority. + * Optional. Integer between 0 and (priorityRange - 1). Specifies the + * priority of the caller if there are no available resources. Lower numbers + * mean higher priority. + * + * @param {Object} options + * Optional. If `options.block` is `false`, immediately return an error if + * no space is available in the queue. * * @returns {boolean} `true` if the pool is not fully utilized, `false` otherwise. */ -Pool.prototype.acquire = function acquire (callback, priority) { +Pool.prototype.acquire = function acquire (callback, priority, options) { if (this._draining) { throw new Error('pool is draining and cannot accept work') } + if (options !== null && typeof options !== "undefined" && options.block === false && this._isFull()) { + callback(new Error('Cannot acquire connection because the pool is full')) + return false + } this._waitingClients.enqueue(callback, priority) this._dispense() return (this._count < this._factory.max) diff --git a/test/generic-pool.test.js b/test/generic-pool.test.js index abecf156..8a4fe0c5 100644 --- a/test/generic-pool.test.js +++ b/test/generic-pool.test.js @@ -461,6 +461,62 @@ module.exports = { }) }, + 'block:false returns error if pool full': function (beforeExit) { + var assertion_count = 0 + var pool = poolModule.Pool({ + name: 'test1', + create: function (callback) { callback({id: Math.floor(Math.random() * 1000)}) }, + destroy: function (client) {}, + max: 2, + idleTimeoutMillis: 100 + }) + pool.acquire(function (err, obj1) { + if (err) { throw err } + pool.acquire(function (err, obj2) { + if (err) { throw err } + pool.acquire(function (err, obj3) { + assert.equal(err.message, 'Cannot acquire connection because the pool is full') + assertion_count += 1 + }, 0, {block: false}) + }, 0, {block: false}) + }) + + beforeExit(function () { + assert.equal(assertion_count, 1) + }) + }, + + // 0. Set poolSize to 2 + // 1. Acquire an object + // 2. Acquire 2nd object (resource created) + // 3. Release one object (all resources created, only one live) + // 4. Acquire an object (should succeed despite resources being created) + 'block:false returns error if all objects created, pool not full': function (beforeExit) { + var assertion_count = 0 + var pool = poolModule.Pool({ + name: 'test-object-created', + create: function (callback) { callback({id: Math.floor(Math.random() * 1000)}) }, + destroy: function (client) {}, + max: 2, + idleTimeoutMillis: 100 + }) + pool.acquire(function (err, obj1) { + if (err) { throw err } + pool.acquire(function (err, obj2) { + if (err) { throw err } + pool.release(obj1) + pool.acquire(function (err, obj3) { + assert.equal(err, null) + assertion_count += 1 + }, 0, {block: false}) + }, 0, {block: false}) + }) + + beforeExit(function () { + assert.equal(assertion_count, 1) + }) + }, + 'availableObjectsCount': function (beforeExit) { var assertion_count = 0 var pool = poolModule.Pool({ From f4ab42c64ae14c8a3a262a52dd8e4d33e9c0c249 Mon Sep 17 00:00:00 2001 From: Kevin Burke Date: Tue, 16 Feb 2016 16:24:48 -0800 Subject: [PATCH 2/6] Support timing out acquire() call The `options` dictionary for pool.acquire() now takes a second argument, `timeout`. Pass an integer number of milliseconds to cancel the acquire() call and hit the callback with pool.Full after the given amount of time has elapsed. Hits the callback with an immediate error if the timeout is a negative number, zero, or NaN. Adds a number of tests that this logic behaves as expected. This work was sponsored by [Shyp](https://shyp.com). --- lib/generic-pool.js | 51 +++++++- test/generic-pool.test.js | 242 +++++++++++++++++++++++++++++++++++++- 2 files changed, 285 insertions(+), 8 deletions(-) diff --git a/lib/generic-pool.js b/lib/generic-pool.js index a3939271..e79143a5 100644 --- a/lib/generic-pool.js +++ b/lib/generic-pool.js @@ -383,8 +383,12 @@ Pool.prototype._isFull = function() { * mean higher priority. * * @param {Object} options - * Optional. If `options.block` is `false`, immediately return an error if - * no space is available in the queue. + * Optional. If `options.block` is `false`, and no space is available in the + * queue, immediately return Pool.full (an Error object). If `options.block` + * is true and `options.timeout` is set to a value greater than 0, attempt + * to enqueue the item for `options.timeout` ms. If the item has not been + * dequeued by that time, hit the callback with Pool.full, and don't run the + * callback. * * @returns {boolean} `true` if the pool is not fully utilized, `false` otherwise. */ @@ -392,9 +396,43 @@ Pool.prototype.acquire = function acquire (callback, priority, options) { if (this._draining) { throw new Error('pool is draining and cannot accept work') } - if (options !== null && typeof options !== "undefined" && options.block === false && this._isFull()) { - callback(new Error('Cannot acquire connection because the pool is full')) - return false + var that = this + if (options !== null && typeof options !== "undefined") { + if (options.block === false) { + if (this._isFull()) { + callback(fullError) + return false + } + } else { + if (typeof options.timeout === "number") { + if (options.timeout <= 0 || isNaN(options.timeout)) { + callback(new Error("Timeout set to immediate or negative value: " + options.timeout)) + return false + } else { + var fired = false + var canceled = false + // This would break if Node ever allowed multiple threads, since + // there's a race between checking (fired) and hitting the callback. + var cbCopy = callback + callback = function(err, obj) { + if (canceled) { + that.release(obj) + return + } + fired = true + cbCopy(err, obj) + } + setTimeout(function() { + if (fired) { + return + } else { + canceled = true + cbCopy(fullError) + } + }, options.timeout) + } + } + } } this._waitingClients.enqueue(callback, priority) this._dispense() @@ -574,4 +612,7 @@ Pool.prototype.getMinPoolSize = function getMinPoolSize () { return this._factory.min } +var fullError = new Error("Cannot acquire resource because the pool is full") + +exports.full = fullError exports.Pool = Pool diff --git a/test/generic-pool.test.js b/test/generic-pool.test.js index 8a4fe0c5..a5548e5e 100644 --- a/test/generic-pool.test.js +++ b/test/generic-pool.test.js @@ -464,7 +464,7 @@ module.exports = { 'block:false returns error if pool full': function (beforeExit) { var assertion_count = 0 var pool = poolModule.Pool({ - name: 'test1', + name: 'block-false', create: function (callback) { callback({id: Math.floor(Math.random() * 1000)}) }, destroy: function (client) {}, max: 2, @@ -474,14 +474,50 @@ module.exports = { if (err) { throw err } pool.acquire(function (err, obj2) { if (err) { throw err } - pool.acquire(function (err, obj3) { - assert.equal(err.message, 'Cannot acquire connection because the pool is full') + pool.acquire(function (err) { + assert.equal(err.message, 'Cannot acquire resource because the pool is full') + assertion_count += 1 + assert.equal(err, poolModule.full) assertion_count += 1 }, 0, {block: false}) }, 0, {block: false}) }) beforeExit(function () { + assert.equal(assertion_count, 2) + }) + }, + + 'block:true, timeout:5 times out if pool full': function (beforeExit) { + var released = false + var assertion_count = 0 + var createCount = 0 + var pool = poolModule.Pool({ + name: 'timeout5', + create: function (callback) { + callback(null, { count: ++createCount }) + }, + destroy: function (client) {}, + max: 2, + idleTimeoutMillis: 100, + }) + pool.acquire(function (err, obj1) { + if (err) { throw err } + pool.acquire(function (err, obj2) { + if (err) { throw err } + setTimeout(function() { + pool.release(obj1) + released = true + }, 10) + pool.acquire(function (err, obj3) { + assert.equal(err, poolModule.full) + assertion_count += 1 + }, 0, {block: true, timeout: 5}) + }, 0, {block: false}) + }) + + beforeExit(function () { + assert(released) assert.equal(assertion_count, 1) }) }, @@ -517,6 +553,206 @@ module.exports = { }) }, + 'block:true, timeout:5 times out at the timeout time, not the release time': function (beforeExit) { + var released = false + var assertion_count = 0 + var createCount = 0 + var pool = poolModule.Pool({ + name: 'timeout5', + create: function (callback) { + callback(null, { count: ++createCount }) + }, + destroy: function (client) {}, + max: 2, + idleTimeoutMillis: 100, + }) + pool.acquire(function (err, obj1) { + if (err) { throw err } + pool.acquire(function (err, obj2) { + if (err) { throw err } + setTimeout(function() { + pool.release(obj1) + released = true + }, 50) + var start = Date.now() + pool.acquire(function (err, obj3) { + // time out times aren't exact, but let's check this is closer to + // 5 than 50 + assert((Date.now() - start) < 10) + assertion_count += 1 + assert.equal(err, poolModule.full) + assertion_count += 1 + }, 0, {block: true, timeout: 5}) + }, 0, {block: false}) + }) + + beforeExit(function () { + assert(released) + assert.equal(assertion_count, 2) + }) + }, + + 'block:true, timeout:5 lets you acquire more resources afterwards': function (beforeExit) { + var released = false + var assertion_count = 0 + var createCount = 0 + var pool = poolModule.Pool({ + name: 'timeout5-release', + create: function (callback) { + callback(null, { count: ++createCount }) + }, + destroy: function (client) {}, + max: 2, + idleTimeoutMillis: 100, + }) + pool.acquire(function (err, obj1) { + if (err) { throw err } + pool.acquire(function (err, obj2) { + if (err) { throw err } + setTimeout(function() { + pool.release(obj1) + released = true + }, 30) + pool.acquire(function (err, obj3) { + assert.equal(err, poolModule.full) + assertion_count += 1 + pool.acquire(function (err, obj4) { + assert.equal(err, null) + assertion_count += 1 + assert.equal(obj4.count, 1) + assertion_count += 1 + }) + }, 0, {block: true, timeout: 5}) + }, 0, {block: false}) + }) + + beforeExit(function () { + assert(released) + assert.equal(assertion_count, 3) + }) + }, + + 'timeout: NaN returns an error': function (beforeExit) { + var assertion_count = 0 + var pool = poolModule.Pool({ + name: 'timeout5', + create: function (callback) { + callback(null, { count: ++createCount }) + }, + destroy: function (client) {}, + max: 2, + }) + pool.acquire(function (err, obj1) { + assert(err.message, "Timeout set to immediate or negative value: NaN") + assertion_count++ + }, 0, {block: true, timeout: NaN}) + beforeExit(function() { + assert.equal(assertion_count, 1) + }) + }, + + 'timeout: 0 returns an error': function (beforeExit) { + var assertion_count = 0 + var pool = poolModule.Pool({ + name: 'timeout5', + create: function (callback) { + callback(null, { count: ++createCount }) + }, + destroy: function (client) {}, + max: 2, + }) + pool.acquire(function (err, obj1) { + assert(err.message, "Timeout set to immediate or negative value: 0") + assertion_count++ + }, 0, {block: true, timeout: 0}) + beforeExit(function() { + assert.equal(assertion_count, 1) + }) + }, + + 'timeout: -1 returns an error': function (beforeExit) { + var assertion_count = 0 + var pool = poolModule.Pool({ + name: 'timeout5', + create: function (callback) { + callback(null, { count: ++createCount }) + }, + destroy: function (client) {}, + max: 2, + }) + pool.acquire(function (err, obj1) { + assert(err.message, "Timeout set to immediate or negative value: -1") + assertion_count++ + }, 0, {block: true, timeout: -1}) + beforeExit(function() { + assert.equal(assertion_count, 1) + }) + }, + + 'block:true, timeout:5 returns resource if pool isnt full': function (beforeExit) { + var assertion_count = 0 + var createCount = 0 + var pool = poolModule.Pool({ + name: 'timeout5', + create: function (callback) { + callback(null, { count: ++createCount }) + }, + destroy: function (client) {}, + max: 2, + idleTimeoutMillis: 100 + }) + pool.acquire(function (err, obj1) { + if (err) { throw err } + pool.acquire(function (err, obj2) { + if (err) { throw err } + assert.equal(err, null) + assertion_count += 1 + assert.equal(obj2.count, 2) + assertion_count += 1 + }, 0, {block: true, timeout: 5}) + }) + + beforeExit(function () { + assert.equal(assertion_count, 2) + }) + }, + + 'block:true, timeout:5 returns resource if it becomes available before timeout': function (beforeExit) { + var released = false + var assertion_count = 0 + var createCount = 0 + var pool = poolModule.Pool({ + name: 'timeout5', + create: function (callback) { + callback(null, { count: ++createCount }) + }, + destroy: function (client) {}, + max: 2, + idleTimeoutMillis: 100 + }) + pool.acquire(function (err, obj1) { + if (err) { throw err } + pool.acquire(function (err, obj2) { + if (err) { throw err } + setTimeout(function() { + pool.release(obj1) + released = true + }, 10) + pool.acquire(function (err, obj3) { + assert.equal(err, null) + assertion_count += 1 + assert.equal(obj3.count, 1) + assertion_count += 1 + }, 0, {block: true, timeout: 500}) + }, 0, {block: false}) + }) + + beforeExit(function () { + assert(released) + assert.equal(assertion_count, 2) + }) + }, + 'availableObjectsCount': function (beforeExit) { var assertion_count = 0 var pool = poolModule.Pool({ From f652f63710baa14ba94b3f30d0ace3879f0505c8 Mon Sep 17 00:00:00 2001 From: Kevin Burke Date: Sat, 20 Feb 2016 11:16:28 -0800 Subject: [PATCH 3/6] Bump timeout to 20ms Apparently the timer resolution isn't fine enough; I've observed this value as high as 22ms, but I just ran the tests ~100 times with the diff set to 20ms. If it's a problem I can bump higher or we can try to figure out some way to force the ordering here with fake timers. --- test/generic-pool.test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/generic-pool.test.js b/test/generic-pool.test.js index a5548e5e..7ac97ecf 100644 --- a/test/generic-pool.test.js +++ b/test/generic-pool.test.js @@ -578,7 +578,8 @@ module.exports = { pool.acquire(function (err, obj3) { // time out times aren't exact, but let's check this is closer to // 5 than 50 - assert((Date.now() - start) < 10) + var diff = Date.now() - start + assert(diff < 20, "expected time difference to be < 15, was " + diff) assertion_count += 1 assert.equal(err, poolModule.full) assertion_count += 1 From a0c5bb2fdd474dc138a833a547621ee578dd8851 Mon Sep 17 00:00:00 2001 From: Kevin Burke Date: Thu, 17 Mar 2016 12:24:30 -0700 Subject: [PATCH 4/6] Remove block option Replace block: false with timeout: 0, update code and tests. I will rebase this down before it's ready for merge. --- lib/generic-pool.js | 69 +++++++++++++++++++-------------------- test/generic-pool.test.js | 63 +++++++++++++---------------------- 2 files changed, 56 insertions(+), 76 deletions(-) diff --git a/lib/generic-pool.js b/lib/generic-pool.js index e79143a5..28292153 100644 --- a/lib/generic-pool.js +++ b/lib/generic-pool.js @@ -383,12 +383,13 @@ Pool.prototype._isFull = function() { * mean higher priority. * * @param {Object} options - * Optional. If `options.block` is `false`, and no space is available in the - * queue, immediately return Pool.full (an Error object). If `options.block` - * is true and `options.timeout` is set to a value greater than 0, attempt - * to enqueue the item for `options.timeout` ms. If the item has not been - * dequeued by that time, hit the callback with Pool.full, and don't run the - * callback. + * Optional. If `options.timeout` is set to a non-negative number, throw an + * error if we cannot acquire a client in `options.timeout` ms. If + * `options.timeout` is 0, we will hit the callback in the next tick if no + * clients are available. If no clients are available in the specified time, + * generic-pool will return Pool.full. + * + * Omit the timeout or set it to `null` to wait indefinitely. * * @returns {boolean} `true` if the pool is not fully utilized, `false` otherwise. */ @@ -398,39 +399,37 @@ Pool.prototype.acquire = function acquire (callback, priority, options) { } var that = this if (options !== null && typeof options !== "undefined") { - if (options.block === false) { - if (this._isFull()) { - callback(fullError) + if (typeof options.timeout === "number") { + if (options.timeout < 0 || isNaN(options.timeout)) { + callback(new Error("Timeout set to negative or invalid value: " + options.timeout)) return false - } - } else { - if (typeof options.timeout === "number") { - if (options.timeout <= 0 || isNaN(options.timeout)) { - callback(new Error("Timeout set to immediate or negative value: " + options.timeout)) + } else if (options.timeout === 0) { + if (this._isFull()) { + callback(fullError) return false - } else { - var fired = false - var canceled = false - // This would break if Node ever allowed multiple threads, since - // there's a race between checking (fired) and hitting the callback. - var cbCopy = callback - callback = function(err, obj) { - if (canceled) { - that.release(obj) - return - } - fired = true - cbCopy(err, obj) + } + } else { + var fired = false + var canceled = false + // This would break if Node ever allowed multiple threads, since + // there's a race between checking (fired) and hitting the callback. + var cbCopy = callback + callback = function(err, obj) { + if (canceled) { + that.release(obj) + return } - setTimeout(function() { - if (fired) { - return - } else { - canceled = true - cbCopy(fullError) - } - }, options.timeout) + fired = true + cbCopy(err, obj) } + setTimeout(function() { + if (fired) { + return + } else { + canceled = true + cbCopy(fullError) + } + }, options.timeout) } } } diff --git a/test/generic-pool.test.js b/test/generic-pool.test.js index 7ac97ecf..8f14b546 100644 --- a/test/generic-pool.test.js +++ b/test/generic-pool.test.js @@ -461,7 +461,7 @@ module.exports = { }) }, - 'block:false returns error if pool full': function (beforeExit) { + 'timeout:0 returns error if pool full': function (beforeExit) { var assertion_count = 0 var pool = poolModule.Pool({ name: 'block-false', @@ -479,8 +479,8 @@ module.exports = { assertion_count += 1 assert.equal(err, poolModule.full) assertion_count += 1 - }, 0, {block: false}) - }, 0, {block: false}) + }, 0, {timeout: 0}) + }, 0, {timeout: 0}) }) beforeExit(function () { @@ -488,7 +488,7 @@ module.exports = { }) }, - 'block:true, timeout:5 times out if pool full': function (beforeExit) { + 'timeout:5 times out if pool full': function (beforeExit) { var released = false var assertion_count = 0 var createCount = 0 @@ -512,8 +512,8 @@ module.exports = { pool.acquire(function (err, obj3) { assert.equal(err, poolModule.full) assertion_count += 1 - }, 0, {block: true, timeout: 5}) - }, 0, {block: false}) + }, 0, {timeout: 5}) + }, 0, {timeout: 0}) }) beforeExit(function () { @@ -527,7 +527,7 @@ module.exports = { // 2. Acquire 2nd object (resource created) // 3. Release one object (all resources created, only one live) // 4. Acquire an object (should succeed despite resources being created) - 'block:false returns error if all objects created, pool not full': function (beforeExit) { + 'timeout:0 returns error if all objects created, pool not full': function (beforeExit) { var assertion_count = 0 var pool = poolModule.Pool({ name: 'test-object-created', @@ -544,8 +544,8 @@ module.exports = { pool.acquire(function (err, obj3) { assert.equal(err, null) assertion_count += 1 - }, 0, {block: false}) - }, 0, {block: false}) + }, 0, {timeout: 0}) + }, 0, {timeout: 0}) }) beforeExit(function () { @@ -553,7 +553,7 @@ module.exports = { }) }, - 'block:true, timeout:5 times out at the timeout time, not the release time': function (beforeExit) { + 'timeout:5 times out at the timeout time, not the release time': function (beforeExit) { var released = false var assertion_count = 0 var createCount = 0 @@ -583,8 +583,8 @@ module.exports = { assertion_count += 1 assert.equal(err, poolModule.full) assertion_count += 1 - }, 0, {block: true, timeout: 5}) - }, 0, {block: false}) + }, 0, {timeout: 5}) + }, 0, {timeout: 0}) }) beforeExit(function () { @@ -593,7 +593,7 @@ module.exports = { }) }, - 'block:true, timeout:5 lets you acquire more resources afterwards': function (beforeExit) { + 'timeout:5 lets you acquire more resources afterwards': function (beforeExit) { var released = false var assertion_count = 0 var createCount = 0 @@ -623,8 +623,8 @@ module.exports = { assert.equal(obj4.count, 1) assertion_count += 1 }) - }, 0, {block: true, timeout: 5}) - }, 0, {block: false}) + }, 0, {timeout: 5}) + }, 0, {timeout: 0}) }) beforeExit(function () { @@ -646,26 +646,7 @@ module.exports = { pool.acquire(function (err, obj1) { assert(err.message, "Timeout set to immediate or negative value: NaN") assertion_count++ - }, 0, {block: true, timeout: NaN}) - beforeExit(function() { - assert.equal(assertion_count, 1) - }) - }, - - 'timeout: 0 returns an error': function (beforeExit) { - var assertion_count = 0 - var pool = poolModule.Pool({ - name: 'timeout5', - create: function (callback) { - callback(null, { count: ++createCount }) - }, - destroy: function (client) {}, - max: 2, - }) - pool.acquire(function (err, obj1) { - assert(err.message, "Timeout set to immediate or negative value: 0") - assertion_count++ - }, 0, {block: true, timeout: 0}) + }, 0, {timeout: NaN}) beforeExit(function() { assert.equal(assertion_count, 1) }) @@ -684,13 +665,13 @@ module.exports = { pool.acquire(function (err, obj1) { assert(err.message, "Timeout set to immediate or negative value: -1") assertion_count++ - }, 0, {block: true, timeout: -1}) + }, 0, {timeout: -1}) beforeExit(function() { assert.equal(assertion_count, 1) }) }, - 'block:true, timeout:5 returns resource if pool isnt full': function (beforeExit) { + 'timeout:5 returns resource if pool isnt full': function (beforeExit) { var assertion_count = 0 var createCount = 0 var pool = poolModule.Pool({ @@ -710,7 +691,7 @@ module.exports = { assertion_count += 1 assert.equal(obj2.count, 2) assertion_count += 1 - }, 0, {block: true, timeout: 5}) + }, 0, {timeout: 5}) }) beforeExit(function () { @@ -718,7 +699,7 @@ module.exports = { }) }, - 'block:true, timeout:5 returns resource if it becomes available before timeout': function (beforeExit) { + 'timeout:5 returns resource if it becomes available before timeout': function (beforeExit) { var released = false var assertion_count = 0 var createCount = 0 @@ -744,8 +725,8 @@ module.exports = { assertion_count += 1 assert.equal(obj3.count, 1) assertion_count += 1 - }, 0, {block: true, timeout: 500}) - }, 0, {block: false}) + }, 0, {timeout: 500}) + }, 0, {timeout: null}) }) beforeExit(function () { From 07f325946d13ec009146cb198e140c89a07e94bd Mon Sep 17 00:00:00 2001 From: Kevin Burke Date: Thu, 17 Mar 2016 12:29:43 -0700 Subject: [PATCH 5/6] Hit the error callback in the next tick --- lib/generic-pool.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/generic-pool.js b/lib/generic-pool.js index 28292153..30efffc3 100644 --- a/lib/generic-pool.js +++ b/lib/generic-pool.js @@ -405,7 +405,9 @@ Pool.prototype.acquire = function acquire (callback, priority, options) { return false } else if (options.timeout === 0) { if (this._isFull()) { - callback(fullError) + process.nextTick(function() { + callback(fullError) + }) return false } } else { From 5556a81ccab1906ef93b836764ec8ee511dbb902 Mon Sep 17 00:00:00 2001 From: Kevin Burke Date: Sat, 26 Mar 2016 19:56:11 -0700 Subject: [PATCH 6/6] Fix lint errors --- lib/generic-pool.js | 16 ++++++++-------- test/generic-pool.test.js | 30 ++++++++++++++++-------------- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/lib/generic-pool.js b/lib/generic-pool.js index 30efffc3..b89174ab 100644 --- a/lib/generic-pool.js +++ b/lib/generic-pool.js @@ -365,7 +365,7 @@ Pool.prototype._ensureMinimum = function _ensureMinimum () { } } -Pool.prototype._isFull = function() { +Pool.prototype._isFull = function () { return this._count >= this._factory.max && this._availableObjects.length === 0 } @@ -398,14 +398,14 @@ Pool.prototype.acquire = function acquire (callback, priority, options) { throw new Error('pool is draining and cannot accept work') } var that = this - if (options !== null && typeof options !== "undefined") { - if (typeof options.timeout === "number") { + if (options !== null && typeof options !== 'undefined') { + if (typeof options.timeout === 'number') { if (options.timeout < 0 || isNaN(options.timeout)) { - callback(new Error("Timeout set to negative or invalid value: " + options.timeout)) + callback(new Error('Timeout set to negative or invalid value: ' + options.timeout)) return false } else if (options.timeout === 0) { if (this._isFull()) { - process.nextTick(function() { + process.nextTick(function () { callback(fullError) }) return false @@ -416,7 +416,7 @@ Pool.prototype.acquire = function acquire (callback, priority, options) { // This would break if Node ever allowed multiple threads, since // there's a race between checking (fired) and hitting the callback. var cbCopy = callback - callback = function(err, obj) { + callback = function (err, obj) { if (canceled) { that.release(obj) return @@ -424,7 +424,7 @@ Pool.prototype.acquire = function acquire (callback, priority, options) { fired = true cbCopy(err, obj) } - setTimeout(function() { + setTimeout(function () { if (fired) { return } else { @@ -613,7 +613,7 @@ Pool.prototype.getMinPoolSize = function getMinPoolSize () { return this._factory.min } -var fullError = new Error("Cannot acquire resource because the pool is full") +var fullError = new Error('Cannot acquire resource because the pool is full') exports.full = fullError exports.Pool = Pool diff --git a/test/generic-pool.test.js b/test/generic-pool.test.js index 8f14b546..5a2eed93 100644 --- a/test/generic-pool.test.js +++ b/test/generic-pool.test.js @@ -499,13 +499,13 @@ module.exports = { }, destroy: function (client) {}, max: 2, - idleTimeoutMillis: 100, + idleTimeoutMillis: 100 }) pool.acquire(function (err, obj1) { if (err) { throw err } pool.acquire(function (err, obj2) { if (err) { throw err } - setTimeout(function() { + setTimeout(function () { pool.release(obj1) released = true }, 10) @@ -564,13 +564,13 @@ module.exports = { }, destroy: function (client) {}, max: 2, - idleTimeoutMillis: 100, + idleTimeoutMillis: 100 }) pool.acquire(function (err, obj1) { if (err) { throw err } pool.acquire(function (err, obj2) { if (err) { throw err } - setTimeout(function() { + setTimeout(function () { pool.release(obj1) released = true }, 50) @@ -579,7 +579,7 @@ module.exports = { // time out times aren't exact, but let's check this is closer to // 5 than 50 var diff = Date.now() - start - assert(diff < 20, "expected time difference to be < 15, was " + diff) + assert(diff < 25, 'expected time difference to be < 25, was ' + diff) assertion_count += 1 assert.equal(err, poolModule.full) assertion_count += 1 @@ -604,13 +604,13 @@ module.exports = { }, destroy: function (client) {}, max: 2, - idleTimeoutMillis: 100, + idleTimeoutMillis: 100 }) pool.acquire(function (err, obj1) { if (err) { throw err } pool.acquire(function (err, obj2) { if (err) { throw err } - setTimeout(function() { + setTimeout(function () { pool.release(obj1) released = true }, 30) @@ -635,38 +635,40 @@ module.exports = { 'timeout: NaN returns an error': function (beforeExit) { var assertion_count = 0 + var createCount = 0 var pool = poolModule.Pool({ name: 'timeout5', create: function (callback) { callback(null, { count: ++createCount }) }, destroy: function (client) {}, - max: 2, + max: 2 }) pool.acquire(function (err, obj1) { - assert(err.message, "Timeout set to immediate or negative value: NaN") + assert(err.message, 'Timeout set to immediate or negative value: NaN') assertion_count++ }, 0, {timeout: NaN}) - beforeExit(function() { + beforeExit(function () { assert.equal(assertion_count, 1) }) }, 'timeout: -1 returns an error': function (beforeExit) { var assertion_count = 0 + var createCount = 0 var pool = poolModule.Pool({ name: 'timeout5', create: function (callback) { callback(null, { count: ++createCount }) }, destroy: function (client) {}, - max: 2, + max: 2 }) pool.acquire(function (err, obj1) { - assert(err.message, "Timeout set to immediate or negative value: -1") + assert(err.message, 'Timeout set to immediate or negative value: -1') assertion_count++ }, 0, {timeout: -1}) - beforeExit(function() { + beforeExit(function () { assert.equal(assertion_count, 1) }) }, @@ -716,7 +718,7 @@ module.exports = { if (err) { throw err } pool.acquire(function (err, obj2) { if (err) { throw err } - setTimeout(function() { + setTimeout(function () { pool.release(obj1) released = true }, 10)