AlinsRan commented on code in PR #13855:
URL: https://github.com/apache/apisix/pull/13855#discussion_r3869707719
##########
apisix/admin/standalone.lua:
##########
@@ -343,9 +346,12 @@ function _M.init_worker()
end
else
local last_modified = config[METADATA_LAST_MODIFIED]
- if last_modified_per_worker ~= last_modified then
+ local digest = config[METADATA_DIGEST]
+ if last_modified_per_worker ~= last_modified
+ or digest_per_worker ~= digest then
update_config(config)
Review Comment:
Fair point — the digest comparison is the half that makes the worker
recover, and nothing in the test touches it. TEST 1 only covers the
`ip_port.lua` crash.
On the shape though: the two fields are not symmetric, so the scenario as
described cannot be produced deterministically from a test. `X-Digest` is the
client header, but `X-Last-Modified` is server-generated
(`apisix_yaml[METADATA_LAST_MODIFIED] = ngx_time()`), so to get two pushes with
the same timestamp both have to land inside one second *and* the poll has to
run between them. That is a race in both directions — it can pass with the fix
reverted and fail with it in place.
What I think is deterministic: write the second config straight into the
`standalone-config` shared dict, keeping the timestamp, changing only the
digest and the content, and posting no event — the polling loop is then the
only thing that can deliver it. Something like:
```lua
local dict = ngx.shared["standalone-config"]
local stored = core.json.decode(dict:get("config"))
stored["X-Digest"] = "changed"
stored.routes_conf_version = stored.routes_conf_version + 1 -- update()
bumps this on every push
stored.routes = { { id = "r2", uri = "/r2", upstream = ... } }
dict:set("config", core.json.encode(stored))
```
with one poll interval allowed to pass first, so the worker has recorded the
previous timestamp — otherwise the second config is its first observation and
would be applied even without the fix.
I have not pushed that test yet: I could not get it to pass locally even
with the fix in place, and I do not want to add a test I have not seen work.
The route pushed in the preceding block was not reachable in my runs, which
looks like a local harness/events problem rather than the code, so I would
rather resolve that first than guess. Will follow up.
Two things worth recording while this is open:
- Comparing both fields rather than the digest alone is deliberate. The
digest is client-supplied and a client may reuse one for different content;
keeping the timestamp in the comparison is the conservative side.
- "Same digest, different content" cannot reach the poll loop at all:
`update()` answers 204 and stores nothing when the digest matches, so that
combination never gets written.
--
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]
