From 2a99b3449ddd573d7e65c81b70dbe7f7d6778d12 Mon Sep 17 00:00:00 2001 From: Tim Robinson Date: Thu, 28 Apr 2016 10:10:39 +0000 Subject: [PATCH 1/8] Fix memory leaks in rate limiter --- node_modules/c9/ratelimit.js | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/node_modules/c9/ratelimit.js b/node_modules/c9/ratelimit.js index 404c6dfa..01c5cd65 100644 --- a/node_modules/c9/ratelimit.js +++ b/node_modules/c9/ratelimit.js @@ -6,26 +6,24 @@ var error = require("http-error"); module.exports = ratelimit; function ratelimit(key, duration, max) { - var limit = {}; + var counts = {}; return function(req, res, next) { var handle = req.params[key]; - var lim = limit[handle] || (limit[handle] = []); - var now = Date.now(); - for (var i = 0; i < lim.length; i++) { - if (now - lim[i] > duration) { - lim.splice(i, 1); - i--; - } - else break; - } - if (lim.length > max) { + counts[handle] = counts[handle] || 0; + if (counts[handle] >= max) { var err = new error.TooManyRequests("Rate limit exceeded"); - err.retryIn = duration - (Date.now() - lim[0]); - next(err); - return; + err.retryIn = Math.min(duration, 5000); + return next(err); } - lim.push(Date.now()); + + counts[handle]++; + setTimeout(function () { + counts[handle]--; + if (counts[handle] == 0) { + delete counts[handle]; + } + }, duration); return next(); }; From 07e7c55b44af54f5894a0fcc75e0fcf0c3648f98 Mon Sep 17 00:00:00 2001 From: Tim Robinson Date: Thu, 28 Apr 2016 10:18:19 +0000 Subject: [PATCH 2/8] Add test --- node_modules/c9/ratelimit_test.js | 33 +++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 node_modules/c9/ratelimit_test.js diff --git a/node_modules/c9/ratelimit_test.js b/node_modules/c9/ratelimit_test.js new file mode 100644 index 00000000..aaa2b2bc --- /dev/null +++ b/node_modules/c9/ratelimit_test.js @@ -0,0 +1,33 @@ +"use server"; + +require("c9/inline-mocha")(module); + +var ratelimit = require("./ratelimit"); +var assert = require("assert"); + +describe("ratelimit", function() { + it("Should limit based on key", function (done) { + var limiter = ratelimit("test", 10, 1); + limiter({params: {test: 1}}, null, function (err) { + assert(!err, err); + limiter({params: {test: 1}}, null, function (err) { + assert(err); + assert.equal(err.code, 429); + done(); + }); + }); + }); + + it("Should work again after a delay", function (done) { + var limiter = ratelimit("test", 10, 1); + limiter({params: {test: 1}}, null, function (err) { + assert(!err, err); + setTimeout(function() { + limiter({params: {test: 1}}, null, function (err) { + assert(!err, err); + done(); + }); + }, 15); + }); + }); +}); \ No newline at end of file From 6c0737c3c08201453b2fe4e48e9d5b4a064ceb30 Mon Sep 17 00:00:00 2001 From: Tim Robinson Date: Thu, 12 May 2016 17:19:31 +0000 Subject: [PATCH 3/8] Switch rate limited to use one setInterval instead of many setTimeouts to clean itself up. --- node_modules/c9/ratelimit.js | 32 ++++++++++++++++--------- node_modules/c9/ratelimit_test.js | 40 ++++++++++++++++++++++++------- 2 files changed, 53 insertions(+), 19 deletions(-) diff --git a/node_modules/c9/ratelimit.js b/node_modules/c9/ratelimit.js index 01c5cd65..0efc15ef 100644 --- a/node_modules/c9/ratelimit.js +++ b/node_modules/c9/ratelimit.js @@ -1,4 +1,5 @@ var error = require("http-error"); +var _ = require("lodash"); /** * In memory rate limiter as connect middleware @@ -6,25 +7,34 @@ var error = require("http-error"); module.exports = ratelimit; function ratelimit(key, duration, max) { - var counts = {}; + var requests = {}; + setInterval(function() { + _.forIn(requests, function (requestsForHandle, key) { + var totalToSplice = 0; + var expireTime = Date.now() - duration; + /* Requests are already sorted by date as they are appended, so we just loop + until we find one that shouldn't have expired and splice them from the list */ + for (var i = 0; i < requestsForHandle.length; i++) { + if (requestsForHandle[i] >= expireTime) break; + totalToSplice++; + } + requests[key].splice(0, totalToSplice); + if (requests[key].length == 0) { + delete requests[key]; + } + }); + }, 10); return function(req, res, next) { var handle = req.params[key]; - counts[handle] = counts[handle] || 0; - if (counts[handle] >= max) { + requests[handle] = requests[handle] || []; + if (requests[handle].length >= max) { var err = new error.TooManyRequests("Rate limit exceeded"); err.retryIn = Math.min(duration, 5000); return next(err); } - counts[handle]++; - setTimeout(function () { - counts[handle]--; - if (counts[handle] == 0) { - delete counts[handle]; - } - }, duration); - + requests[handle].push(Date.now()); return next(); }; } diff --git a/node_modules/c9/ratelimit_test.js b/node_modules/c9/ratelimit_test.js index aaa2b2bc..6277de4f 100644 --- a/node_modules/c9/ratelimit_test.js +++ b/node_modules/c9/ratelimit_test.js @@ -4,13 +4,14 @@ require("c9/inline-mocha")(module); var ratelimit = require("./ratelimit"); var assert = require("assert"); +var async = require("async"); describe("ratelimit", function() { it("Should limit based on key", function (done) { - var limiter = ratelimit("test", 10, 1); - limiter({params: {test: 1}}, null, function (err) { + var limiter = ratelimit("username", 10, 1); + limiter({params: {username: "super"}}, null, function (err) { assert(!err, err); - limiter({params: {test: 1}}, null, function (err) { + limiter({params: {username: "super"}}, null, function (err) { assert(err); assert.equal(err.code, 429); done(); @@ -18,16 +19,39 @@ describe("ratelimit", function() { }); }); - it("Should work again after a delay", function (done) { - var limiter = ratelimit("test", 10, 1); - limiter({params: {test: 1}}, null, function (err) { + it("Should work again after a delay", function (done) { + var limiter = ratelimit("username", 10, 1); + limiter({params: {username: "super"}}, null, function (err) { assert(!err, err); setTimeout(function() { - limiter({params: {test: 1}}, null, function (err) { + limiter({params: {username: "super"}}, null, function (err) { assert(!err, err); done(); }); - }, 15); + }, 25); }); }); + + it("Should work with many requests", function (done) { + var MAX_REQUESTS = 5; + var limiter = ratelimit("username", 10, MAX_REQUESTS); + var successfulRequests = 0; + async.times(10, function(n, next) { + limiter({params: {username: "super"}}, null, function (err) { + if (err) return next(err); + successfulRequests++; + next(); + }); + }, function (err) { + assert.equal(successfulRequests, MAX_REQUESTS); + setTimeout(function() { + limiter({params: {username: "super"}}, null, function (err) { + assert(!err, err); + done(); + }); + }, 25); + }); + }); + + }); \ No newline at end of file From 9d6c7e47d71ef0ccfea9a7477ace6bef90990086 Mon Sep 17 00:00:00 2001 From: Tim Robinson Date: Thu, 12 May 2016 17:27:48 +0000 Subject: [PATCH 4/8] Added tests for multiple keys and ensuring the expiry system is working as intended --- node_modules/c9/ratelimit_test.js | 47 +++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/node_modules/c9/ratelimit_test.js b/node_modules/c9/ratelimit_test.js index 6277de4f..fd674350 100644 --- a/node_modules/c9/ratelimit_test.js +++ b/node_modules/c9/ratelimit_test.js @@ -7,6 +7,7 @@ var assert = require("assert"); var async = require("async"); describe("ratelimit", function() { + it("Should limit based on key", function (done) { var limiter = ratelimit("username", 10, 1); limiter({params: {username: "super"}}, null, function (err) { @@ -19,6 +20,19 @@ describe("ratelimit", function() { }); }); + it("Should work with different keys", function (done) { + var limiter = ratelimit("username", 10, 1); + limiter({params: {username: "super"}}, null, function (err) { + assert(!err, err); + limiter({params: {username: "aloha"}}, null, function (err) { + assert(!err, err); + done(); + }); + }); + }); + + + it("Should work again after a delay", function (done) { var limiter = ratelimit("username", 10, 1); limiter({params: {username: "super"}}, null, function (err) { @@ -53,5 +67,38 @@ describe("ratelimit", function() { }); }); + it("Should expire keys at the correct times", function (done) { + var limiter = ratelimit("username", 50, 2); + async.series([ + function(next) { + limiter({params: {username: "mario"}}, null, function(err) { + assert(!err, err); + setTimeout(next, 25); + }); + }, + function (next) { + limiter({params: {username: "mario"}}, null, function(err) { + assert(!err, err); + setTimeout(next, 40); + }); + }, + function (next) { + limiter({params: {username: "mario"}}, null, function(err) { + assert(!err, err); + limiter({params: {username: "mario"}}, null, function(err) { + assert(err); + assert.equal(err.code, 429); + setTimeout(next, 20); + }); + }); + }, + function (next) { + limiter({params: {username: "mario"}}, null, function(err) { + assert(!err, err); + next(); + }); + } + ], done); + }); }); \ No newline at end of file From 1e04331410d7c9e1a723cc87aa62204d78cb44e9 Mon Sep 17 00:00:00 2001 From: Tim Robinson Date: Mon, 16 May 2016 19:15:15 +0000 Subject: [PATCH 5/8] Made requests object safer. Expire requests before sending error back --- node_modules/c9/ratelimit.js | 58 ++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 25 deletions(-) diff --git a/node_modules/c9/ratelimit.js b/node_modules/c9/ratelimit.js index 0efc15ef..33f9d47c 100644 --- a/node_modules/c9/ratelimit.js +++ b/node_modules/c9/ratelimit.js @@ -7,34 +7,42 @@ var _ = require("lodash"); module.exports = ratelimit; function ratelimit(key, duration, max) { - var requests = {}; + var requests = Object.create(null); // in case there handles like 'constructor' setInterval(function() { - _.forIn(requests, function (requestsForHandle, key) { - var totalToSplice = 0; - var expireTime = Date.now() - duration; - /* Requests are already sorted by date as they are appended, so we just loop - until we find one that shouldn't have expired and splice them from the list */ - for (var i = 0; i < requestsForHandle.length; i++) { - if (requestsForHandle[i] >= expireTime) break; - totalToSplice++; - } - requests[key].splice(0, totalToSplice); - if (requests[key].length == 0) { - delete requests[key]; - } - }); - }, 10); - return function(req, res, next) { - var handle = req.params[key]; - - requests[handle] = requests[handle] || []; - if (requests[handle].length >= max) { - var err = new error.TooManyRequests("Rate limit exceeded"); - err.retryIn = Math.min(duration, 5000); - return next(err); + Object.keys(requests).forEach(expireRequests); + }, duration * 0.75); + + function expireRequests(value) { + var requestsForHandle = requests[value]; + var totalToSplice = 0; + var expireTime = Date.now() - duration; + /* Requests are already sorted by date as they are appended, so we just loop + until we find one that shouldn't have expired and splice them from the list */ + for (var i = 0; i < requestsForHandle.length; i++) { + if (requestsForHandle[i] >= expireTime) break; + totalToSplice++; + } + requests[value].splice(0, totalToSplice); + if (requests[value].length == 0) { + delete requests[value]; } - requests[handle].push(Date.now()); + return true; + } + + return function(req, res, next) { + var value = req.params[key]; + + requests[value] = requests[value] || []; + if (requests[value].length >= max) { + if (expireRequests(value) && requests[value].length >= max) { + var err = new error.TooManyRequests("Rate limit exceeded"); + err.retryIn = Math.min(duration, 5000); + return next(err); + } + } + + requests[value].push(Date.now()); return next(); }; } From 833f3d1911fc07a888dd85ac28b2309e94677801 Mon Sep 17 00:00:00 2001 From: Tim Robinson Date: Mon, 16 May 2016 19:24:49 +0000 Subject: [PATCH 6/8] rename back to handle --- node_modules/c9/ratelimit.js | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/node_modules/c9/ratelimit.js b/node_modules/c9/ratelimit.js index 33f9d47c..a56f6c3e 100644 --- a/node_modules/c9/ratelimit.js +++ b/node_modules/c9/ratelimit.js @@ -12,8 +12,8 @@ function ratelimit(key, duration, max) { Object.keys(requests).forEach(expireRequests); }, duration * 0.75); - function expireRequests(value) { - var requestsForHandle = requests[value]; + function expireRequests(handle) { + var requestsForHandle = requests[handle]; var totalToSplice = 0; var expireTime = Date.now() - duration; /* Requests are already sorted by date as they are appended, so we just loop @@ -22,27 +22,27 @@ function ratelimit(key, duration, max) { if (requestsForHandle[i] >= expireTime) break; totalToSplice++; } - requests[value].splice(0, totalToSplice); - if (requests[value].length == 0) { - delete requests[value]; + requests[handle].splice(0, totalToSplice); + if (requests[handle].length == 0) { + delete requests[handle]; } return true; } return function(req, res, next) { - var value = req.params[key]; + var handle = req.params[key]; - requests[value] = requests[value] || []; - if (requests[value].length >= max) { - if (expireRequests(value) && requests[value].length >= max) { + requests[handle] = requests[handle] || []; + if (requests[handle].length >= max) { + if (expireRequests(handle) && requests[handle].length >= max) { var err = new error.TooManyRequests("Rate limit exceeded"); err.retryIn = Math.min(duration, 5000); return next(err); } } - requests[value].push(Date.now()); + requests[handle].push(Date.now()); return next(); }; } From ccc8b1b56ec5c1f3340dfddd507a37c4d35bfb4d Mon Sep 17 00:00:00 2001 From: Tim Robinson Date: Mon, 16 May 2016 19:28:39 +0000 Subject: [PATCH 7/8] Iterate just one variable --- node_modules/c9/ratelimit.js | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/node_modules/c9/ratelimit.js b/node_modules/c9/ratelimit.js index a56f6c3e..0b3cdcd3 100644 --- a/node_modules/c9/ratelimit.js +++ b/node_modules/c9/ratelimit.js @@ -18,9 +18,8 @@ function ratelimit(key, duration, max) { var expireTime = Date.now() - duration; /* Requests are already sorted by date as they are appended, so we just loop until we find one that shouldn't have expired and splice them from the list */ - for (var i = 0; i < requestsForHandle.length; i++) { - if (requestsForHandle[i] >= expireTime) break; - totalToSplice++; + for (totalToSplice = 0; totalToSplice < requestsForHandle.length; totalToSplice++) { + if (requestsForHandle[totalToSplice] >= expireTime) break; } requests[handle].splice(0, totalToSplice); if (requests[handle].length == 0) { From a2e0ee24bcdd1121bfc6a7c3ce94318bc094fb4e Mon Sep 17 00:00:00 2001 From: Tim Robinson Date: Wed, 18 May 2016 08:22:56 -0400 Subject: [PATCH 8/8] Remove expiring on every failed request, add Max expire duration. --- node_modules/c9/ratelimit.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/node_modules/c9/ratelimit.js b/node_modules/c9/ratelimit.js index 0b3cdcd3..48997420 100644 --- a/node_modules/c9/ratelimit.js +++ b/node_modules/c9/ratelimit.js @@ -1,6 +1,8 @@ var error = require("http-error"); var _ = require("lodash"); +var MAX_EXPIRE_INTERVAL = 5000; + /** * In memory rate limiter as connect middleware */ @@ -10,7 +12,7 @@ function ratelimit(key, duration, max) { var requests = Object.create(null); // in case there handles like 'constructor' setInterval(function() { Object.keys(requests).forEach(expireRequests); - }, duration * 0.75); + }, Math.min(duration * 0.75, MAX_EXPIRE_INTERVAL)); function expireRequests(handle) { var requestsForHandle = requests[handle]; @@ -34,11 +36,9 @@ function ratelimit(key, duration, max) { requests[handle] = requests[handle] || []; if (requests[handle].length >= max) { - if (expireRequests(handle) && requests[handle].length >= max) { - var err = new error.TooManyRequests("Rate limit exceeded"); - err.retryIn = Math.min(duration, 5000); - return next(err); - } + var err = new error.TooManyRequests("Rate limit exceeded"); + err.retryIn = Math.min(duration, 5000); + return next(err); } requests[handle].push(Date.now());