From 70df5a3a16050a9a80ac50b23aa13aea354a2272 Mon Sep 17 00:00:00 2001
From: Michal Cichra <michal@cichra.cz>
Date: Mon, 3 Apr 2017 07:42:30 +0200
Subject: [PATCH] [balancer] fix memory leak in round-robin balancer

before:

/016-balancer.t .. TEST 1: round robin does not leak memory
WARNING: method GET not supported for ab when taking a request body
LeakTest: [5768 5964 5964 6192 6224 6224 6500 6500 6500 6760 6788 6788 6928 7088 7088 7088 7532 8300 8300 8300 8300 8300 8856 9672 10084 10084 10084 10084 10084 10584 10584 10584 10584 10584 10776 11112 11112 11112 11112 11112 11460 11668 11668 11668 11668 11668 11872 12256 12256 12256 12256 12256 12256 12732 12872 12872 13128 14664 14664 14664 14664 14664 14664 14664 14664 15168 15968 16740 17500 18088 18164 18164 18164 18164 18164 18164 18164 18164 18164 18164 18460 19080 19144 19144 19144 19144 19144 19144 19144 19144 19144 19144 19144 19636 20164 20164 20164 20164 20164 20164]
LeakTest: k=158.8
t/016-balancer.t .. ok
All tests successful.
Files=1, Tests=4,  6 wallclock secs ( 0.03 usr  0.00 sys +  1.21 cusr  3.91 csys =  5.15 CPU)
Result: PASS

after:

t/016-balancer.t .. TEST 1: round robin does not leak memory
WARNING: method GET not supported for ab when taking a request body
LeakTest: [2248 2248 2248 2248 2248 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252 2252]
LeakTest: k=0.0
t/016-balancer.t .. ok
All tests successful.
Files=1, Tests=4,  6 wallclock secs ( 0.02 usr  0.01 sys +  1.15 cusr  3.55 csys =  4.73 CPU)
Result: PASS
---
 CHANGELOG.md                               |  1 +
 apicast/src/resty/balancer/round_robin.lua | 24 +++++------
 t/016-balancer.t                           | 46 ++++++++++++++++++++++
 3 files changed, 59 insertions(+), 12 deletions(-)
 create mode 100644 t/016-balancer.t

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 32fb2867b..85a515127 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -27,6 +27,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Memory leak in every request [PR #339](https://github.com/3scale/apicast/pull/339)
 - Remove unnecessary code and comments [PR #344](https://github.com/3scale/apicast/pull/344)
 - JWT expiry not taken into account in authorization response cache [PR #283](https://github.com/3scale/apicast/pull/283) / [Issue #309](https://github.com/3scale/apicast/issues/309) / Fixed by [PR #341](https://github.com/3scale/apicast/pull/341)
+- Memory leak in round robin balancer [PR #345](https://github.com/3scale/apicast/pull/345)
 
 ## [3.0.0-beta3] - 2017-03-20
 
diff --git a/apicast/src/resty/balancer/round_robin.lua b/apicast/src/resty/balancer/round_robin.lua
index b9b301a0a..be7c46d8c 100644
--- a/apicast/src/resty/balancer/round_robin.lua
+++ b/apicast/src/resty/balancer/round_robin.lua
@@ -3,25 +3,26 @@ local min = math.min
 local random = math.random
 
 local balancer = require 'resty.balancer'
-local semaphore = require 'ngx.semaphore'
 local balancer_random = require 'resty.balancer.random'
+local lrucache = require 'resty.lrucache'
 
 local _M = {
-  _VERSION = '0.1'
+  _VERSION = '0.1',
+  cache_size = 1000
 }
 
-local call = semaphore.new(1)
-
-local cursor = {}
+local cursor
 
 function _M.new()
   return balancer.new(_M.call)
 end
 
 function _M.reset()
-  cursor = {}
+  cursor = lrucache.new(_M.cache_size)
 end
 
+_M.reset()
+
 function _M.call(peers)
   if #peers == 0 then
     return nil, 'empty peers'
@@ -33,15 +34,14 @@ function _M.call(peers)
     return balancer_random.call(peers)
   end
 
-  local ok, _ = call:wait(0)
-
+  -- This looks like there might be a race condition but I think it is not.
+  -- The VM will not schedule another thread until there is IO and in this case there is no IO.
+  -- So this block stays the only one being executed and no one can access the same cursor value.
   local n = #peers
-  local i = min(max(cursor[hash] or peers.cur or random(1, n), 0), n)
+  local i = min(max(cursor:get(hash) or peers.cur or random(1, n), 0), n)
   local peer = peers[i]
 
-  cursor[hash] = (i % n) + 1
-
-  if ok then call:post(1) end
+  cursor:set(hash, (i % n) + 1)
 
   return peer, i
 end
diff --git a/t/016-balancer.t b/t/016-balancer.t
new file mode 100644
index 000000000..f50462524
--- /dev/null
+++ b/t/016-balancer.t
@@ -0,0 +1,46 @@
+use Test::Nginx::Socket 'no_plan';
+use Cwd qw(cwd);
+
+my $pwd = cwd();
+my $apicast = $ENV{TEST_NGINX_APICAST_PATH} || "$pwd/apicast";
+
+$ENV{TEST_NGINX_LUA_PATH} = "$apicast/src/?.lua;;";
+$ENV{TEST_NGINX_HTTP_CONFIG} = "$apicast/http.d/*.conf";
+$ENV{TEST_NGINX_APICAST_PATH} = $apicast;
+$ENV{RESOLVER} = '127.0.1.1:5353';
+
+
+env_to_nginx(
+    'RESOLVER'
+);
+master_on();
+log_level('debug');
+repeat_each(1);
+no_root_location();
+run_tests();
+
+__DATA__
+
+=== TEST 1: round robin does not leak memory
+Balancing different hosts does not leak memory.
+--- http_config
+  lua_package_path "$TEST_NGINX_LUA_PATH";
+  init_by_lua_block {
+    require('resty.balancer.round_robin').cache_size = 1
+  }
+--- config
+  location = /t {
+    content_by_lua_block {
+      local round_robin = require('resty.balancer.round_robin')
+      local balancer = round_robin.new()
+
+      local peers = { hash = ngx.var.request_id, cur = 1,  1, 2 }
+      local peer = round_robin.call(peers)
+
+      ngx.print(peer)
+    }
+  }
+--- pipelined_requests eval
+[ "GET /t", "GET /t" ]
+--- response_body eval
+[ "1", "1" ]