keep the key never overrides #6
Conversation
never overrides the (least recently used) unexpired items in the store when running out of storage in the shared memory zone
|
I think it's better to expose an option to let the user decide whether to use |
its more strictness for some special case
|
@agentzh please take a look at first, add the test case later. |
add the test case openresty#14: serial lock and unlock with safe_add
| @@ -154,7 +156,7 @@ function _M.lock(self, key) | |||
| elapsed = elapsed + step | |||
| timeout = timeout - step | |||
|
|
|||
| local ok, err = dict:add(key, true, exptime) | |||
| local ok, err = self.dict_add(dict, key, true, exptime) | |||
There was a problem hiding this comment.
Better cache self.dict_add in a local variable since it may get called for multiple times later on in this method.
use safe_add, dont have enough memory
| [error] | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Style: let's remove these file trailing new lines :)
There was a problem hiding this comment.
There is 3 blank lines between different test cases. Do i need to remove the line #503 ?
There was a problem hiding this comment.
@membphis These 3 blank lines were at the end of the file. And only in that case, we should remove them :)
There was a problem hiding this comment.
fine, i have fixed this before.
modify the format for the source and test case.
| --- request | ||
| GET /t | ||
| --- response_body | ||
| lock1: nil, no memory |
There was a problem hiding this comment.
Hmm, I think we need a clone of this test case except safe_add = false to ensure there is a difference.
There was a problem hiding this comment.
In addition, we need another clone of this test case without explicitly setting the safe_add option to ensure that the default behaviour is safe_add = false.
three case: use the safe_add to “true” , “false” or default, confirm it’ll work correct.
|
|
||
|
|
||
|
|
||
| === TEST 17: use safe_add by default, dont have enough memory |
There was a problem hiding this comment.
I think the following title is better:
the "safe_add" option is off by default: exhausting the shm zone memory
Your title looks ambiguous to me.
| @@ -107,6 +108,7 @@ function _M.new(_, dict_name, opts) | |||
| local self = { | |||
| cdata = cdata, | |||
| dict = dict, | |||
| dict_add = safe_add and dict.safe_add or dict.add, | |||
There was a problem hiding this comment.
Code readability is not good enough.
how about this:
local dict_add = dict.add
if safe_add then
dict_add = dict.safe_add
end
If the `auto_ssl` shared dict ran out of memory, and then nginx were reloaded, then a race condition existed where multiple "sockproc" processes could try to be started at the same time. While I think this situation would be unlikely to affect a production system in a negative way (since the race condition only occurs during nginx reloads and 1 of the sockproc processes should still succeed and allow things to work), it did lead to some errors being logged, which would intermittently cause our tests to fail (it would crop up on the test following our t/memory.t test, since that next test would be the first reload following our test to explicitly exhaust the memory). This is fixed by using the `auto_ssl_settings` shared dict for storing the resty-lock details (the lock prevents multiple processes from being started at once). This smaller shared dict (introduced in #68) is used for storing bits of data that won't grow in size so we can better ensure the data will never be evicted from the cache. I'm now able to repeatedly run the test suite in a loop without hitting this edge case. Note that we are still using the `auto_ssl` shared dict for storing resty-lock details for domain registrations, since the memory requirements for that may grow (since there's a lock per domain, it's dynamic in size). But that should be okay, because similar to the SSL certs stored in `auto_ssl`, we're okay with cache evictions for old data in those cases (along with warnings being logged). Also possibly relevant is that currently resty-lock always uses `add` for the shared dict (so it will evict old data to make room if necessary), but there's a pull request for allowing use of `safe_add`: openresty/lua-resty-lock#6 While we should be okay by switching things to `auto_ssl_settings` (since we should never have enough stored data in there to need evicting old data), it's something to keep an eye on. Related to: - #68 (comment) - #48 (comment)
never overrides the (least recently used) unexpired items in the store
when running out of storage in the shared memory zone