Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cache-locks #1402

Merged
merged 6 commits into from
Jul 23, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions kong/templates/nginx_kong.lua
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ lua_shared_dict cache ${{MEM_CACHE_SIZE}};
lua_shared_dict reports_locks 100k;
lua_shared_dict cluster_locks 100k;
lua_shared_dict cluster_autojoin_locks 100k;
lua_shared_dict cache_locks 100k;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we do not need so many different *_locks shared dicts. A single one is able to hold multiple locks with different options (expiration, timeout, etc...). It will be worth cleaning this up some time soon.

lua_shared_dict cassandra 1m;
lua_shared_dict cassandra_prepared 5m;
lua_socket_log_errors off;
Expand Down
Empty file added kong/tools/07-cache
Empty file.
32 changes: 27 additions & 5 deletions kong/tools/database_cache.lua
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
local resty_lock = require "resty.lock"
local cjson = require "cjson"
local cache = ngx.shared.cache

Expand Down Expand Up @@ -120,21 +121,42 @@ function _M.all_apis_by_dict_key()
end

function _M.get_or_set(key, cb)
local lock = resty_lock:new("cache_locks", {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resty_lock:new can return an error ("dictionary not found" when missing the shm). It should be handled too.

exptime = 10,
timeout = 10
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the default value of 5 for timeout is safer. If anything this value should probably be decreased down to 1 or even 0, but not increased, especially since get_or_set() is called in the hotest code path of our application (in the resolver's find_api. timeout prevents a call to lock or unlock from hanging too long.

Secondly: the lock should not be created unless the case hit is a miss. Currently, this lock is created before even trying to access the cache (a few lines down).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the lock should not be created unless the case hit is a miss.

I am handling this case. Before creating the lock we have:

value = _M.get(key)
if value then return value end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By creating the lock I mean :new(), not lock()

})

local value, err
-- Try to get

-- Try to get the value from the cache
value = _M.get(key)
if value then return value end

-- The value is missing, acquire a lock
local elapsed, err = lock:lock(key)
if not elapsed then
ngx.log(ngx.ERR, "failed to acquire cache lock: "..err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer using a comma:

ngx.log(ngx.ERR, "failed to acquire cache lock: ", err)

It is less expensive to call ngx.log in its variadic form rather than performing the string concatenation on the Lua-land side, which is more expansive.

end

-- Lock acquired. Since in the meantime another worker may have
-- populated the value we have to check again
value = _M.get(key)
if not value then
-- Get from closure
value, err = cb()
if err then
return nil, err
elseif value then
if value then
local ok, err = _M.set(key, value)
if not ok then
if err then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to stay consistent in our error handling, all across our application. I believe the previous condition if not ok then was a good fit. additionally, this change won't pass the linting because ok is unused. If your API (_M.set()) returns false without an error, then this variable doesn't have a proper name. Since it is called ok, we assume it is the standard Lua pattern of having the first return value indicating success, thus if not ok then is fine, and preferred.

ngx.log(ngx.ERR, err)
end
end
end

local ok, err = lock:unlock()
if not ok then
ngx.log(ngx.ERR, "failed to unlock: "..err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto: concatenation

end

return value
end

Expand Down
84 changes: 84 additions & 0 deletions spec/02-integration/07-cache/database_cache_spec.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
local helpers = require "spec.helpers"
local cjson = require "cjson"
local meta = require "kong.meta"

describe("Resolver", function()
local client
setup(function()
helpers.kill_all()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to call kill_all() when setting up tests anymore. Tests are responsible for doing this in their teardown phase.


assert(helpers.dao.apis:insert {
request_host = "mockbin.com",
upstream_url = "http://mockbin.com"
})

assert(helpers.start_kong({
["custom_plugins"] = "database-cache",
lua_package_path = "?/init.lua;./kong/?.lua;./spec/fixtures/?.lua"
}))

-- Add the plugin
local admin_client = helpers.admin_client()
local res = assert(admin_client:send {
method = "POST",
path = "/apis/mockbin.com/plugins/",
body = {
name = "database-cache"
},
headers = {
["Content-Type"] = "application/json"
}
})
assert.res_status(201, res)
admin_client:close()
end)

teardown(function()
helpers.stop_kong()
end)

it("avoids dog-pile effect", function()
local function make_request(premature, sleep_time)
local client = helpers.proxy_client()
local res = assert(client:send {
method = "GET",
path = "/status/200?sleep="..sleep_time,
headers = {
["Host"] = "mockbin.com"
}
})
res:read_body()
client:close()
end

local ok, err = ngx.timer.at(0, make_request, 2)
assert.truthy(ok)

local ok, err = ngx.timer.at(0, make_request, 5)
assert.truthy(ok)

local ok, err = ngx.timer.at(0, make_request, 1)
assert.truthy(ok)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those 3 can basically be replaced with the proper Lua idiom:

assert(ngx.timer.at(0, make_request, 2))
assert(ngx.timer.at(0, make_request, 5))
assert(ngx.timer.at(0, make_request, 1))


helpers.wait_until(function()
local admin_client = helpers.admin_client()
local res = assert(admin_client:send {
method = "GET",
path = "/cache/invocations"
})
local body = res:read_body()
admin_client:close()
return cjson.decode(body).message == 3
end, 10)

-- Invocation are 3, but lookups should be 1
local admin_client = helpers.admin_client()
local res = assert(admin_client:send {
method = "GET",
path = "/cache/lookups"
})
local body = res:read_body()
admin_client:close()
assert.equal(1, cjson.decode(body).message)
end)
end)
35 changes: 35 additions & 0 deletions spec/fixtures/kong/plugins/database-cache/handler.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
local BasePlugin = require "kong.plugins.base_plugin"
local cache = require "kong.tools.database_cache"

local INVOCATIONS = "invocations"
local LOOKUPS = "lookups"

local DatabaseCacheHandler = BasePlugin:extend()

DatabaseCacheHandler.PRIORITY = 1000

function DatabaseCacheHandler:new()
DatabaseCacheHandler.super.new(self, "database-cache")
end

function DatabaseCacheHandler:init_worker()
DatabaseCacheHandler.super.init_worker(self)

cache.rawset(INVOCATIONS, 0)
cache.rawset(LOOKUPS, 0)
end

function DatabaseCacheHandler:access(conf)
DatabaseCacheHandler.super.access(self)

cache.get_or_set("pile_effect", function()
cache.incr(LOOKUPS, 1)
-- Adds some delay
ngx.sleep(tonumber(ngx.req.get_uri_args().sleep))
return true
end)

cache.incr(INVOCATIONS, 1)
end

return DatabaseCacheHandler
3 changes: 3 additions & 0 deletions spec/fixtures/kong/plugins/database-cache/schema.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
return {
fields = {}
}