membphis commented on code in PR #14061:
URL: https://github.com/apache/apisix/pull/14061#discussion_r4373484358
##########
apisix/core/lrucache.lua:
##########
@@ -218,41 +263,86 @@ end
local function refresh_stale_objs()
+ -- snapshot first: creating an object may yield, and the pool can change
+ -- meanwhile (new stale hits, purge_inactive_objs)
+ local pending = {}
for lru_obj, keys in pairs(stale_obj_pool) do
for key, new_obj in pairs(keys) do
- local obj, err = new_obj.fn(unpack(new_obj.args))
- if obj ~= nil then
- lru_obj:set(key, {val = obj, ver = new_obj.ver}, new_obj.ttl)
+ pending[#pending + 1] = {lru_obj, keys, key, new_obj}
+ end
+ end
+
+ for _, item in ipairs(pending) do
+ local lru_obj, keys, key, new_obj = item[1], item[2], item[3], item[4]
+ if keys[key] ~= new_obj then
+ goto continue
+ end
+
+ local ver, ttl = new_obj.ver, new_obj.ttl
+ local obj, err = new_obj.fn(unpack(new_obj.args))
+ if obj ~= nil then
+ -- the item may have been purged while refreshing; writing it back
+ -- then would resurrect it without an access record
+ if keys[key] == new_obj then
+ lru_obj:set(key, {val = obj, ver = ver}, ttl)
keys[key] = nil
log.info("successfully refresh stale obj for key ",
- tostring(key), " to ver ", new_obj.ver)
- else
- log.error("failed to refresh stale obj for key ", key, ": ",
err)
+ tostring(key), " to ver ", ver)
end
+ else
+ log.error("failed to refresh stale obj for key ", key, ": ", err)
end
+
+ ::continue::
end
end
-local function init_worker()
+local function purge_inactive_objs()
+ local now = ngx_now()
+ for lru_obj, inactive_lru in pairs(inactive_lru_pool) do
+ local access_times = inactive_lru.access_times
+ local inactive = inactive_lru.inactive
+ local pending = stale_obj_pool[lru_obj]
+ for key, access_time in pairs(access_times) do
+ if now - access_time > inactive then
+ lru_obj:delete(key)
Review Comment:
[P2] Preserve stale specs that are still referenced by live resources
This deletes a cached spec based only on request inactivity, even when a
live Route still references the same spec URL. It introduces an availability
regression compared with the baseline.
Trigger:
1. Successfully fetch and cache a remote spec for a Route.
2. Keep the Route configured, but send no requests for longer than
`spec_url_ttl`, allowing the purge timer to run. Assume no capacity eviction or
cache rebuild.
3. Make the spec endpoint unavailable, then send an otherwise valid request
to that Route.
Before this PR, the expired entry is still available as a stale object. With
`refresh_stale = true`, the request is validated against that last-known-good
spec and continues to the upstream; a refresh is queued in the background, and
its failure does not fail the current request.
With this PR, `lru_obj:delete(key)` removes that fallback. The next request
synchronously calls `fetch_and_compile()`; if downloading the spec fails,
`oas-validator.access()` returns HTTP 500 before proxying. Even when
downloading succeeds, the request now waits for a remote fetch that previously
ran in the background. The unchanged business Route and upstream therefore
acquire an additional runtime dependency after each idle period.
Please fix this before merging. Reclaim specs only after no effective
resource references their cache key, while preserving stale serving and
background refresh for specs that are still referenced, or use another design
that preserves the same request availability without a synchronous refetch. Add
a regression test covering successful caching, an idle period beyond the TTL,
and a subsequent request while the spec endpoint is unavailable, alongside the
unused-spec reclamation tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
