Repository navigation
Conversation
|
Could you run one of the benchmark to check the impact of the atomic.Pointer on the hot path? |
|
The atomic pointer should be completely free on aarch64 and x86-64. If anything I should benchmark the check though. |
|
+3% (Go) instructions per request. Not terrible, but also not perfect... |
f965bcf to
b7cccf2
Compare
|
instead of old requests being rerouted through new route tree, we now take their existing state and execute it on the new runtime after the reload comes back. should be +/-0 instructions |
| maxThreads int | ||
| requestOptions []RequestOption | ||
| requestChan chan *frankenPHPContext | ||
| done <-chan struct{} |
There was a problem hiding this comment.
It probably would make sense to store and check readiness somewhere on the Server directly and not re-use them across reloads.
Would be a bit of a bigger refactor,.but IMO the behavior should be the same for regular threads and workers.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Retired worker requests can hang after draining stops, and queued regular requests can execute with stale configuration.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds request handoff during FrankenPHP configuration reloads to reduce transient 503 responses.
Changes:
- Drains retired worker queues and propagates runtime errors.
- Redispatches requests to the newly active Caddy app.
- Adds reload, queue-draining, and routing tests.
| File | Description |
|---|---|
worker.go |
Drains retired worker queues and snapshots metrics. |
worker_internal_test.go |
Tests worker queue draining and expiration. |
threadregular_test.go |
Updates completion channel type. |
testdata/reload.php |
Provides reload request fixture. |
scaling.go |
Prevents scaling retired workers. |
options.go |
Adds worker drain timeout option. |
frankenphp.go |
Retires servers before runtime shutdown. |
context.go |
Changes completion channels to carry errors. |
caddy/servername.go |
Resolves hosts through nested routes. |
caddy/serveridx_test.go |
Tests stable reload module mapping. |
caddy/reload.go |
Implements request redispatch and startup waiting. |
caddy/reload_test.go |
Tests reload handoff behavior and limits. |
caddy/module.go |
Invokes handoff on retired servers. |
caddy/app.go |
Tracks the active app and reload identities. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
cbf6d7c to
1765184
Compare
When Caddy swaps its configuration, the new frankenphp app replaces the global PHP runtime while the old HTTP routes keep serving. Requests arriving in that window were answered 503 instead of being handled by the new runtime, and requests parked in worker queues could hang forever. Wait for the replacement app and replay such requests through its own HTTP chain, so its matchers and middleware apply to the request the client sent as if it arrived fresh. Requests parked in the old queues are released back to the module, which replays them; late senders are rejected by a sticky sign bit, and metrics are captured per runtime so they stay balanced across the swap.
1765184 to
e496c90
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Handoff can corrupt responses already started by upstream middleware, and the worker retirement protocol lacks focused concurrency coverage.
2 open findings
2 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| func (worker *worker) retire() { | ||
| const retired = -1 << 31 | ||
| worker.queuedRequests.Add(retired) | ||
| for worker.queuedRequests.Load() != retired { |
The capture records what the client sent and the ingress writer before any configured middleware runs, and the replay goes through the replacement chain on that writer. When the old chain consumed body bytes it kept no copy of, the handoff refuses rather than replay a truncated body.
e496c90 to
b0fc7d9
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Queued regular requests can hang without replacement regular threads, and retired middleware errors can corrupt replayed responses.
3 open findings
1 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
| if fc.server.regularRequestsRetired.Load() { | ||
| fc.done <- ErrNotRunning | ||
| continue |
| body: r.Body, contentLength: r.ContentLength, | ||
| writer: w, guard: guard, | ||
| }) | ||
| return next.ServeHTTP(guard, r) |
|
Hmm this PR has gotten a lot more complex since I last took a look. Is the replaying of the request on the caddy side really necessary? Would it work to just keep a reference to all the old workers and then after reload has finished pass all requests from the old requestChan into the workers (with the same name) from the new requestChan? That would be a bit dangerous to do if the configuration actually meaningfully changed, but I guess the same thing happens here in this PR? |
|
I'm currently looking for other approaches here and kind of use the PR here as a testing ground. Should be converted to draft. |
edit: just a reminder to myself to do it properly after api con
edit: prevent 503s from requests coming in during config reloads (complementary to nicolas' PR #2661)