Repository navigation
Conversation
connect() spawns the catalog and health watchers, waits for the first one and kills both. resty.consul reads the response body through resty.http, which does that in a child coroutine. When the killed watcher was reading a body, ngx.thread.kill does not cancel that child's pending socket read; its completion later resumes connect() at an unrelated yield point, e.g. the receive of the following catalog fetch, which then fails with "bad argument #1 to 'str_sub' (string expected, got boolean)". The watchers only need the status and the X-Consul-Index header. Read them with resty.http request(), which does all its I/O on the calling coroutine, and close the connection without reading the body.
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation addresses the coroutine race while preserving watcher behavior and includes targeted regression coverage.
0 open findings
What changed in this PR
Prevents killed Consul watcher threads from leaving pending body reads that corrupt subsequent discovery requests.
Changes:
- Reworks watchers to read only response status and headers.
- Adds a regression test reproducing the watcher race.
| File | Description |
|---|---|
apisix/discovery/consul/client.lua |
Uses direct header-only HTTP requests for watchers. |
t/discovery/consul-watch-race.t |
Tests concurrent index changes and slow response streaming. |
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Consul discovery's
connect()spawns a catalog watcher and a health watcher, waits for the first one to return and then kills both. The watchers usedresty.consul, which reads the response body through resty.http, and resty.http reads a body in a child coroutine. If the watcher that gets killed is still reading its body (typically the health watch streaming a large/v1/health/state/anyresponse while the catalog watch returns first),ngx.thread.killdoes not cancel the read pending in that child coroutine. When the read completes later, it resumesconnect()at whatever it is waiting on at that moment, usually the status-line receive of the following catalog fetch. That fetch then fails with:and the round is retried with backoff. It is easy to hit when both indexes change together and the health response is large.
The watchers only need the status and the
X-Consul-Indexheader. This PR makes them send the blocking query with resty.httprequest(), which does all of its I/O (connect, send, status line, headers) on the calling coroutine, and close the connection without reading the body. A killed watcher therefore never leaves a pending operation behind in a child coroutine. Theconnect()loop itself (spawn, wait, kill, re-arm, retry/backoff, non-keepalive mode) is unchanged, and the query parameters, token header and timeouts are the same as before.Behavior notes:
The new test
t/discovery/consul-watch-race.truns a mock Consul that bumps both indexes together, answers the catalog watch shortly after the health watch has started streaming a slow chunked body, and delays the non-blocking catalog read so that the health body finishes while the fetch is waiting. Before this change the test fails with thegot booleanerror and the route returns 503; after it, the services are fetched and the route returns 200.Which issue(s) this PR fixes:
N/A
Checklist