Use lock in limit req

[hnakamur]

First of all, thanks for sharing a nice library!

I noticed a FIXME comment lib/resty/limit/req.lua and this pull request is my attempt to fix it.

Could you review this? Thanks!

[doujiang24]

@hnakamur thanks for your contribution :) I think using resty.lock here is too heavy, we can implement another sharedict API for this case, like getset.

[hnakamur]

Thanks, another sharedict API getset sounds nice for other purposes too! Could you elaborate that?

I suppose the syntax like below

syntax: success, err, forcible = ngx.shared.DICT:getset(key, update_func, exptime?, flags?)
context: Same as set

Sets a key-value pair into the shm-based dictionary with the value calculated returned by update_func. update_func is a function which takes the old value as the first argument and
and returns the new value.

Is this correct?

[hnakamur]

update_func can err as the second return value.

new_value, err = update_func(old_value)

if err ~= nil then no set will be performed on the shm-based dictionary and the value for the key remains the old_value.

[doujiang24]

@hnakamur I mean the redis style getset, old_value, err = getset(key, new_value). yeah, this only can not avoid the race completely, but we can see if there is a race with this new API when old_value ~= latest_value, we can reimburse it then.

to avoid the race completely, we may need another API, like ok, err = setwhen(key, new_value, old_value), this means the value will be updated when the current value is old_value.

[hnakamur]

Thanks, but in the case of incoming method of resty.limit.req, we need to get old_value first in order to calculate new_value, right?

local old_value = get(key)
local new_value = some calculation from old_value
ok, err = setwhen(key, new_value, old_value)

So I think getset is not needed, just setwhen is.

But how about adding a method named update with the syntax as I commented?

local ok, err = update(key, function (old_value)
    return some calculation from old_value
end)

In this way, just one method call is needed, and locking the shared dict and looking up the node in the tree of the shared dict is executed just once, not twice, so I think it is more performant.

[doujiang24]

@hnakamur yeah, getset is not needed when we had setwhen.

for your update suggestion, it's not safe enough to callback Lua function during the shdict operation since we will use mutex lock inside the operation, prue C should be better.

[hnakamur]

Thanks for your comment. I'm not sure but it is safe if we unlock the mutex properly, isn't it? Could you give me more detailed explanation about that it's not safe enough?

I've refined the update method syntax since then and just finished writing the implementation, test codes and the document for lua-nginx-module to see if it actually works. Could you take a look?

https://github.com/openresty/lua-nginx-module/compare/master...hnakamur:add_shdict_update

If it is on the right track, I will add the FFI version of update method and add it also to stream-lua-nginx-module and openresty/lua-resty-core: New FFI-based API for lua-nginx-module.

[hnakamur]

Ah, I see, if you access the same dictionary in the lua function called from the update method, then it will be stuck, right?

Then I throw away the update method and try writing the set_when method.

[hnakamur]

But I think it is enough to tell that don't access the shared dictionary in new_value_func function in the CAUTION section of the document.

If set_when fails, we must restart from get, so we need to write get and set_when in a loop, and I think it's too tedious.

What do you think?

[hnakamur]

I've added an explanation of deadlock to the CAUTION section.

It may not be safe, but I think it's easy to spot when you actually write code to cause dead lock, as it does not work as expected and that is very easy to notice.

I think that the race condition caused by successive calls of get and set is more problematic, as it sometimes works as expected and sometimes it doesn't.

[hnakamur]

I modified resty.limit.req to use ngx.shared.DICT.update and verified tests succeed.

https://github.com/openresty/lua-resty-limit-traffic/compare/master...hnakamur:fix_race_condition_with_shdict_update_method

[hnakamur]

I also made set_when version. This version passes tests too.

https://github.com/openresty/lua-nginx-module/compare/master...hnakamur:add_shdict_set_when

https://github.com/openresty/lua-resty-limit-traffic/compare/master...hnakamur:fix_race_condition_with_shdict_update_method

[hnakamur]

I wrote set_when for stream-lua-nginx-module and FFI version for both lua-nginx-module and stream-lua-nginx-module too.

• https://github.com/openresty/stream-lua-nginx-module/compare/master...hnakamur:shdict_set_when
• https://github.com/openresty/lua-resty-core/compare/master...hnakamur:add_shdict_set_when

[hnakamur]

And after re-reading FFI Semantics, now I think that I should avoid update method since it is a callback style API.

[hnakamur]

I made another pull request Fix race condition in resty.limit.req using ngx.shared.DICT.set_when by hnakamur · Pull Request #49 · openresty/lua-resty-limit-traffic, so I'm closing this now.