Merge pull request +13692 from c9/all-improve-ratelimiter

Fix memory leaks in rate limiter

node_modules/c9/ratelimit.js

@@ -1,4 +1,7 @@
 var error = require("http-error");
+var _ = require("lodash");
+
+var MAX_EXPIRE_INTERVAL = 5000;
 
 /**
  * In memory rate limiter as connect middleware
@@ -6,27 +9,39 @@ var error = require("http-error");
 module.exports = ratelimit;
 
 function ratelimit(key, duration, max) {
-    var limit = {};
+    var requests = Object.create(null); // in case there handles like 'constructor'
+    setInterval(function() {
+        Object.keys(requests).forEach(expireRequests);
+    }, Math.min(duration * 0.75, MAX_EXPIRE_INTERVAL));
+    
+    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 
+            until we find one that shouldn't have expired and splice them from the list */
+        for (totalToSplice = 0; totalToSplice < requestsForHandle.length; totalToSplice++)  {
+            if (requestsForHandle[totalToSplice] >= expireTime) break;
+        }
+        requests[handle].splice(0, totalToSplice);
+        if (requests[handle].length == 0) {
+            delete requests[handle];
+        }
+        
+        return true;
+    }
+    
     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) {
+        requests[handle] = requests[handle] || [];
+        if (requests[handle].length >= 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());
         
+        requests[handle].push(Date.now());
         return next();
     };
 }

node_modules/c9/ratelimit_test.js

@@ -0,0 +1,104 @@
+"use server";
+
+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("username", 10, 1);
+        limiter({params: {username: "super"}}, null, function (err) {
+            assert(!err, err);
+            limiter({params: {username: "super"}}, null, function (err) {
+                assert(err);
+                assert.equal(err.code, 429);
+                done();
+            });
+        });
+    });
+    
+    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) {
+            assert(!err, err);
+            setTimeout(function() {
+                limiter({params: {username: "super"}}, null, function (err) {
+                    assert(!err, err);
+                    done();
+                });
+            }, 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);
+        });
+    });
+    
+    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);
+    });
+    
+});