Copilot commented on code in PR #14061:
URL: https://github.com/apache/apisix/pull/14061#discussion_r4334522752
##########
t/plugin/oas-validator3.t:
##########
@@ -562,3 +573,244 @@ status: 200
}
--- response_body
passed
+
+
+
+=== TEST 24: changing spec_url on a route releases the previously compiled spec
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin")
+ local http = require("resty.http")
+ local fetches = ngx.shared.oas_spec_fetches
+
+ local function put_route(id, spec_url)
+ local code, body = t.test('/apisix/admin/routes/' .. id,
ngx.HTTP_PUT, {
+ uri = "/api/v3/pet",
+ host = "r" .. id .. ".test",
+ plugins = {["oas-validator"] = {spec_url = spec_url}},
+ upstream = {type = "roundrobin", nodes =
{["127.0.0.1:1970"] = 1}},
+ })
+ assert(code < 300, body)
+ ngx.sleep(0.5)
+ end
+
+ local function hit(id)
+ local res, err = http.new():request_uri(
+ "http://127.0.0.1:"; .. ngx.var.server_port ..
"/api/v3/pet", {
+ method = "POST",
+ body = '{"id": 10, "name": "doggie", "photoUrls":
["string"]}',
+ headers = {
+ ["Content-Type"] = "application/json",
+ ["Host"] = "r" .. id .. ".test",
+ },
+ })
+ assert(res and res.status == 200, err or (res and res.status))
+ end
+
+ local function full_gc()
+ collectgarbage()
+ collectgarbage()
+ end
+ put_route(1, "http://127.0.0.1:1979/counted/a.json";)
+ hit(1)
+ put_route(1, "http://127.0.0.1:1979/counted/b.json";)
+ hit(1)
+ full_gc()
+
+ -- nothing references a.json any more, so another route declaring
+ -- it has to fetch it again
+ put_route(2, "http://127.0.0.1:1979/counted/a.json";)
+ hit(2)
+
+ ngx.say("a: ", fetches:get("/counted/a.json"))
+ ngx.say("b: ", fetches:get("/counted/b.json"))
+
+ t.test('/apisix/admin/routes/1', ngx.HTTP_DELETE)
+ t.test('/apisix/admin/routes/2', ngx.HTTP_DELETE)
+ }
+ }
+--- response_body
+a: 2
+b: 1
+
+
+
+=== TEST 25: routes declaring the same spec_url share one compiled spec
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin")
+ local http = require("resty.http")
+ local fetches = ngx.shared.oas_spec_fetches
+
+ local function put_route(id, spec_url)
+ local code, body = t.test('/apisix/admin/routes/' .. id,
ngx.HTTP_PUT, {
+ uri = "/api/v3/pet",
+ host = "r" .. id .. ".test",
+ plugins = {["oas-validator"] = {spec_url = spec_url}},
+ upstream = {type = "roundrobin", nodes =
{["127.0.0.1:1970"] = 1}},
+ })
+ assert(code < 300, body)
+ ngx.sleep(0.5)
+ end
+
+ local function hit(id)
+ local res, err = http.new():request_uri(
+ "http://127.0.0.1:"; .. ngx.var.server_port ..
"/api/v3/pet", {
+ method = "POST",
+ body = '{"id": 10, "name": "doggie", "photoUrls":
["string"]}',
+ headers = {
+ ["Content-Type"] = "application/json",
+ ["Host"] = "r" .. id .. ".test",
+ },
+ })
+ assert(res and res.status == 200, err or (res and res.status))
+ end
+
+ local function full_gc()
+ collectgarbage()
+ collectgarbage()
+ end
Review Comment:
The helper functions `put_route`, `hit`, and `full_gc` are duplicated across
multiple new tests (24–27). This increases maintenance cost and makes future
tweaks (e.g., endpoint, request body, or waiting strategy) error-prone.
Consider defining these helpers once (e.g., at the top of the test file’s Lua
block or via a shared local function factory) and reusing them across the test
cases.
##########
apisix/plugins/oas-validator.lua:
##########
@@ -176,6 +183,22 @@ local function fetch_and_compile(conf)
return validator
end
+local function create_spec_url_entry(conf, cache_key, ttl)
+ local entry = shared_spec_url_entries[cache_key]
+ if entry and ngx_now() - entry.fetched_at < ttl then
+ return entry
+ end
+
+ local validator, err = fetch_and_compile(conf)
+ if not validator then
+ return nil, err
+ end
+
+ entry = {validator = validator, fetched_at = ngx_now()}
+ shared_spec_url_entries[cache_key] = entry
+ return entry
+end
Review Comment:
Concurrent first-use (or concurrent refresh) of the same `cache_key` by
multiple resources can race here: if `shared_spec_url_entries[cache_key]` is
empty/expired, multiple workers/requests can simultaneously call
`fetch_and_compile(conf)`, leading to duplicate fetch/compile work and
potentially large transient memory spikes. Consider serializing creation per
`cache_key` (e.g., singleflight pattern or `resty.lock` keyed by `cache_key`,
or reusing an internal `core.lrucache` keyed by `cache_key` specifically to
gate creation) so only one fetch/compile happens for a given spec at a time.
--
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]
