Skip to content

Commit

Permalink
Race condition when using async.times. fixes #29 by @kp96
Browse files Browse the repository at this point in the history
  • Loading branch information
noamshemesh committed Feb 11, 2017
2 parents af44cd4 + 2c4f277 commit e45864b
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 18 deletions.
2 changes: 1 addition & 1 deletion History.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,4 @@

## v1.0.1 / 2014-03-14

* fix race condition resetting the keys
* fix race condition resetting the keys.
25 changes: 13 additions & 12 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@ function Limiter(opts) {
* @api public
*/

Limiter.prototype.inspect = function () {
return '<Limiter id='
+ this.id + ', duration='
+ this.duration + ', max='
+ this.max + '>';
Limiter.prototype.inspect = function() {
return '<Limiter id=' +
this.id + ', duration=' +
this.duration + ', max=' +
this.max + '>';
};

/**
Expand All @@ -57,7 +57,7 @@ Limiter.prototype.inspect = function () {
* @api public
*/

Limiter.prototype.get = function (fn) {
Limiter.prototype.get = function(fn) {
var count = this.prefix + 'count';
var limit = this.prefix + 'limit';
var reset = this.prefix + 'reset';
Expand All @@ -72,7 +72,7 @@ Limiter.prototype.get = function (fn) {
.set([count, max, 'PX', duration, 'NX'])
.set([limit, max, 'PX', duration, 'NX'])
.set([reset, ex, 'PX', duration, 'NX'])
.exec(function (err, res) {
.exec(function(err, res) {
if (err) return fn(err);

// If the request has failed, it means the values already
Expand Down Expand Up @@ -104,21 +104,22 @@ Limiter.prototype.get = function (fn) {
}

db.multi()
.set([count, n - 1, 'PX', ex * 1000 - dateNow, 'XX'])
.decr(count)
.pexpire([count, ex * 1000 - dateNow])
.pexpire([limit, ex * 1000 - dateNow])
.pexpire([reset, ex * 1000 - dateNow])
.exec(function (err, res) {
.exec(function(err, res) {
if (err) return fn(err);
if (isFirstReplyNull(res)) return mget();
n = n - 1;
n = Array.isArray(res[0]) ? ~~res[0][1] : ~~res[0];
done();
});
}

function mget() {
db.watch([count], function (err) {
db.watch([count], function(err) {
if (err) return fn(err);
db.mget([count, limit, reset], function (err, res) {
db.mget([count, limit, reset], function(err, res) {
if (err) return fn(err);
if (!res[0] && res[0] !== 0) return create();

Expand Down
4 changes: 4 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,12 @@
"limiter",
"limit"
],
"scripts": {
"test": "mocha"
},
"dependencies": {},
"devDependencies": {
"async": "2.1.4",
"ioredis": "1.15.1",
"mocha": "*",
"redis": "2.6.0-1",
Expand Down
39 changes: 34 additions & 5 deletions test/index.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
require('should');
var Limiter = require('..');
var Limiter = require('..'),
async = require('async');

// Uncomment the following line if you want to see
// debug logs from the node-redis module.
//redis.debug_mode = true;

['redis', 'ioredis'].forEach(function (redisModuleName) {
['redis', 'ioredis'].forEach(function(redisModuleName) {
var redisModule = require(redisModuleName);
var db = require(redisModuleName).createClient();
describe('Limiter with ' + redisModuleName, function() {
Expand Down Expand Up @@ -146,7 +147,7 @@ var Limiter = require('..');
.pttl(['limit:something:count'])
.pttl(['limit:something:limit'])
.pttl(['limit:something:reset'])
.exec(function (err, res) {
.exec(function(err, res) {
if (err) return done(err);
var ttlCount = (typeof res[0] === 'number') ? res[0] : res[0][1];
var ttlLimit = (typeof res[1] === 'number') ? res[1] : res[1][1];
Expand Down Expand Up @@ -211,7 +212,8 @@ var Limiter = require('..');

if (err) {
done(err);
} else {
}
else {
responses.forEach(function(res) {
res[1].remaining.should.equal(left < 0 ? 0 : left);
left--;
Expand All @@ -230,7 +232,8 @@ var Limiter = require('..');
limits[0].get(function(err, res) {
if (err) {
done(err);
} else {
}
else {
res.remaining.should.equal(left--);

// Simulate multiple concurrent requests.
Expand All @@ -241,5 +244,31 @@ var Limiter = require('..');
});
});
});

describe('when limiter is called in parallel by multiple clients', function() {
var max = 6,
limiter;

limiter = new Limiter({
duration: 10000,
max: max,
id: 'asyncsomething',
db: redisModule.createClient()
});

it('should set the count properly without race conditions', function(done) {
async.times(max, function(n, next) {
limiter.get(next);
},
function(errs, limits) {

limits.forEach(function(limit) {
limit.remaining.should.equal(max--);
});
done();

});
});
})
});
});

0 comments on commit e45864b

Please sign in to comment.