fix(cache) soft errors would not be handled properly by the cache #3150
Conversation
| for _ = 1, 2 do | ||
| local value, err = cache:get(key, nil, load_into_memory, SOFT_ERROR) | ||
| assert.is_nil(value) | ||
| assert(err:find(SOFT_ERROR.err, 1, true), "expected `" .. tostring(err) .. |
There was a problem hiding this comment.
Probably better to use the native assert.matches(expected, subj, nil, true) here
There was a problem hiding this comment.
Totally missed that assertion, I actually thought about writing one some day. Glad I didn't do that just yet 🙈
| for _ = 1, 2 do | ||
| local value, err = cache:get(key, nil, load_into_memory, HARD_ERROR) | ||
| assert.is_nil(value) | ||
| assert(err:find(HARD_ERROR.err, 1, true), "expected `" .. tostring(err) .. |
| cache = kong_cache.new { | ||
| cluster_events = cluster_events, | ||
| worker_events = worker_events, | ||
| } |
There was a problem hiding this comment.
style: the indentation seems to be completely off in this chunk? :(
kong/cache.lua
Outdated
| -- Temporary fix to convert soft callback errors into hard ones. | ||
| -- FIXME: use upstream mlcache lib instead of local copy | ||
| local soft_to_hard = function(cb) | ||
| return function(...) |
There was a problem hiding this comment.
perf: it would probably be worth it to maintain a cache of such closures to lower the GC overhead and ensure we JIT compile in such hot code paths served by the caching library (performance being the whole point here). This change as it is would be quite hurtful to performance.
thibaultcha
added
pr/changes requested
Tieske
added
pr/status/please review
and removed
pr/changes requested
kong/cache.lua
Outdated
| local function create_wrapper(cb) | ||
| s2h_cache[cb] = function(...) | ||
| local result, err = cb(...) | ||
| if result == nil and err then |
There was a problem hiding this comment.
It is instructed to users of this API that they should never rely on the truthiness/falseness of the first returned value to infer errors (since the library caches misses as well). As such, as a developer I would easily expect that when I write a callback, I can return whichever value I want when the second one is an error (non-nil). While this isn't the typical idiom we try to follow in this codebase (we prefer nil + err), I believe this comes down to usability/robustness: we should only check for non-nil errors here to infer one (disregarding the first return value).
There was a problem hiding this comment.
@thibaultcha all comments addressed
There was a problem hiding this comment.
Looking good! Let's wait for the tests to double check.
kong/cache.lua
Outdated
| @@ -31,6 +31,28 @@ local function log(lvl, ...) | |||
| return ngx_log(lvl, "[DB cache] ", ...) | |||
| end | |||
|
|
|||
| -- Temporary fix to convert soft callback errors into hard ones. | |||
kong/cache.lua
Outdated
| return s2h_cache[cb] | ||
| end | ||
|
|
||
| soft_to_hard = function(cb) |
|
Merged! |
The callback run inside a
pcalland the error checking will only deal with thepcallfailing, not with apcallsuccess and returningnil + err.This is temporary until the mlcache dependency is updated (upstream lib instead of a copy)