fix: request reloads

[AlliBalliBaba]

Alternative to #2648

fixes the lost requests by keeping track of the previous main thread @henderkes

The only thing I'm not sure about is when doing this is save (aka Caddy config has not changed too much on reload).
With this setup all regular requests are passed on and all worker requests are passed on by worker name (no overhead).

[dunglas]

I prefer this approach to #2705 (now closed): it fixes #2698 in the library rather than in the Caddy module, and it also hands over requests queued on draining workers. I ran the reload test from 2705 against this branch and it passes 4/4 (it fails on main with ~186 × 500). It's worth adding here: feaee08 (caddy/admin_test.go + testdata/worker-slow-boot.php).

A few things I'd like to see addressed before merging, details inline:

• servers are never unregistered anymore, so after a final Shutdown() requests block instead of getting ErrNotRunning
• a *Server can't be passed to Init() again after Shutdown(), which breaks library users
• the handoff stops after 5s idle, and requests for workers that disappeared in the new config are never handed over
• threadregular_test.go is deleted, it covered #2612

Also, mainThread is now read on the request path (ServeHTTP, newWorkerDummyContext) while Init() swaps it. It may be worth making it an atomic.Pointer to keep the race detector quiet under load during reloads.

The macOS failure is in the setup-php step, unrelated.

[dunglas]

With unregisterServers() gone, nothing sets isRegistered back to false. After a final Shutdown() (no Init() following, e.g. a library user or Caddy exiting), Server.ServeHTTP queues the request on a channel no thread reads, and it waits forever unless max_wait_time is set. On main it gets ErrNotRunning.

Maybe the handoff could own the unregistration: reQueueRequests() unregisters the old servers once it's done forwarding, and for a final shutdown, the old main thread exposes a retired channel that queued requests select on, so they're rejected with ErrNotRunning if no Init() picks them up.

[dunglas]

time.After is re-armed on each iteration, so this is a 5s idle timeout. Once it fires, a request still routed to the old server (Caddy keeps the old HTTP server up until the whole new config has started, other apps included) queues on the old channel and hangs.

Two more cases end the same way:

• old workers whose name has no match in the new config: nothing reads their requestChan anymore
• newMainThread.regularRequestChan <- fc doesn't select on newMainThread.done, so a shutdown during the handoff leaks the goroutine and the request

Suggestion: forward until the old servers are unregistered instead of a fixed timeout, reject (fc.reject(ErrNotRunning)) requests for unmatched workers, and select on newMainThread.done when sending.

[AlliBalliBaba]

Would you be fine with just forwarding until the next reload happens? The idle goroutines wouldn't consume much resources.

[dunglas]

This turns Init(WithServer(s)) → Shutdown() → Init(WithServer(s)) into an error, since s stays registered (the TestServer change below asserts it). That's the natural reload pattern for library users, and it worked in 1.13.0.

Could a reused server just be re-pointed to the new main thread instead? Build its worker maps off to the side during initWorkers() and swap them in registerServers(), so requests hitting it during boot keep the old routing (and get handed over) until the swap.

[AlliBalliBaba]

Yes I think we never should have allowed registering the same server twice. Not sure how big the BC break would be since this is a new feature for library users. But it would probably be worth it. Currently, servers can leak hidden configuration across re-registrations.

[dunglas]

If reuse is kept (see the comment on options.go), this can go back to asserting that the server serves again after re-Init().

[henderkes]

Alternative to #2648

fixes the lost requests by keeping track of the previous main thread @henderkes

The only thing I'm not sure about is when doing this is save (aka Caddy config has not changed too much on reload).

Yeah that's the reason why the previous PR blew up as much. This is too simple for various reasons when the config changes even in small ways. Examples being added auto, a changed document root on the matched route, removed routes and co. That's why we need some kind of equivalence check between old and new routes before handing a request over.

[henderkes]

Auth*, not auto. In general I agree that this isn't as big of an issue for the normal case, e.g. just reloading the same config or barely changed config again, but the restart window where old connections are still held can be quite long, and I don't want to accidentally introduce the security issues flagged by sloppilot in the last PR. The service restart does a frankenphp reload --force under the hood after all.

[AlliBalliBaba]

WDYT about having a "fingerprint" (in the current diff), that only allows re-queueing requests if all json config for FrankenPHP has not changed

[dunglas]

I would keep things simple and return a 503 for these requests, regardless of if config changed or not.

[AlliBalliBaba]

Hmm rejecting requests is kind of what we wanted to avoid though (from #2698 and #2469).

What do you think about just instructing users to use the http://localhost:2019/frankenphp/workers/restart endpoint for code changes? (does the reload without dropping requests)

I don't think there's a clean way to resolve this considering that PHP can only have one global instance per-process. I'd maybe also advice users to swap out the process, but that kinda goes against the Caddy way.

[henderkes]

503'ing is exactly what we want to avoid.

I think what we can do is do a minimal capture of the caddy side of the request and then replay it against the new configuration. It should be mostly safe and get rid of the 503's when config hasn't changed meaningfully. I'll open a new PR.

[dunglas]

I quite like promoting http://localhost:2019/frankenphp/workers/restart for this use case. It's faster and less intrusive than reloading all Caddy modules anyway.

[AlliBalliBaba]

Completely replaying requests is probably also a bit risky, as you don't know what Caddy modules in front have already done with the request or what they might do if seeing the same requests twice.

I'll try if I can simplify this a bit more. maybe we can also just pass on the request chans to the new workers on reload without config change.

[henderkes]

That may break new deployments e.g. through symlink swapping. We need to do the php runtime swap at least

[AlliBalliBaba]

True, what about calling frankenphp.Stop()/Start() in the Cleanup() of the old app. To make sure all requests have passed though before restarting PHP

[henderkes]

Yes, maybe, but we need to make sure to stall requests incoming during that time. Would sidestep the whole replay and handoff..

[dunglas]

It looks like a cleaner approach to me too.

[henderkes]

I'm having a look, but I'm not sure Caddy actually lets us figure out if there are still incoming requests being handled by its middleware. So I think it would need a small upstream patch to let us observe that.

[AlliBalliBaba]

I tried it and it catches a lot of requests, but it will still drop requests that are already stalled and waiting inside FrankenPHP.