feat: unify php_server logic#2499
Conversation
09cd397 to
39db699
Compare
|
Another subtlety I fixed: the "external workers" from |
henderkes
left a comment
henderkes
left a comment
There was a problem hiding this comment.
I think there's also a problem with DrainWorkers on main at the moment. Seems like it takes the same path with drainPHPThreads as Shutdown logic, but then never sets the state so the php threads could restart. Not directly related to this PR but probably better to fix it right here (or remove the function, it's marked experimental anyways).
|
|
|
+1 for removal because it currently hard-kills the current php runtime with no way to recover it, instead of letting threads perform a graceful reload. |
|
I'll open a separate PR for removing it. First will have to look up why it was added. I think it was for tests initially. |
This PR removes the experimental `DrainWorkers` function since it serves no purpose anymore (as also discussed in #2499). The original use was to have a graceful shutdown of all workers on Caddy shutdown (added [here](https://github.com/php/frankenphp/pull/1405/changes)). Since shutdowns have long been fully safe, the function is not in use anymore. It might only be confusing for library users, since it does not allow recovering drained workers.
|
This refactor is exactly the direction I need for #2398, so I plan to rebase on it once it lands: One small addition would make Motivation: workers are now scoped per server, and cross-server name collisions are resolved with the It would also let #2398's background workers reuse the API-wise the minimal version is a |
|
Having a server name makes sense to me 👍 , the fallback can just be |
|
I opened a few follow up to help get this merged ASAP, if that helps! |
|
@AlliBalliBaba feel free to cherry-pick/squasj my other PRs as you prefer. I'm looking forward to this being merged quickly so let me know if I can help in any way (rebase, etc) |
Follow-up to #2499, targeting `refactor/phpserver`. `Server.isRegistered` is written by `Init()`/`Shutdown()` while in-flight `ServeHTTP()` calls read it concurrently, e.g. during an admin API reload under traffic; the race detector flags the plain bool. This makes it an `atomic.Bool`. The pre-existing global `isRunning` flag has the same pattern, but `Server` is the surface library users now interact with, so it seemed worth fixing here; `isRunning` can be handled separately if wanted.
Follow-up to #2499, targeting `refactor/phpserver`. Test-only. During review, an explicit `server_idx` colliding with an auto-assigned one could make one handler serve another's files; the fix (deferring registration to a single pass keyed by index) had no regression test, and nothing in `caddy/` exercised `server_idx` at all. This adds unit coverage for: - Caddyfile parsing assigns incrementing indexes - modules with the same `server_idx` share one server instance, registered only once - modules without `server_idx` each get their own server - a module with an explicit `server_idx` equal to an already registered one joins the existing server instead of re-registering it (the double-registration scenario possible with JSON configs POSTed to the admin API) These are pure Go unit tests on `registerModules()`; an end-to-end admin API config-swap test would be a natural follow-up but needs the PHP runtime, so it is not included here.
5 participants
Currently the concept of a
php_serveronly exists on the caddy side and not the FrankenPHP side.Lately we have been moving more and more in a direction of scoping requests or workers to specific
php_serverblocks.This PR is an attempt at refactoring the current
php_serverlogic so it is properly mirrored on the FrankenPHP side without BC breaks for library users (and to prevent future bugs like mentioned in #2487)