Skip to content

fix: review follow-ups on the php_server refactor - #2565

Open
nicolas-grekas wants to merge 4 commits into
php:refactor/phpserverfrom
nicolas-grekas:phpserver-fixes
Open

fix: review follow-ups on the php_server refactor#2565
nicolas-grekas wants to merge 4 commits into
php:refactor/phpserverfrom
nicolas-grekas:phpserver-fixes

Conversation

@nicolas-grekas

Copy link
Copy Markdown
Contributor

Follow-up to #2499, targeting refactor/phpserver. Four commits, each independently reviewable.

Findings from a close read of the branch. Nothing here changes the direction of the refactor; the first two are the ones I would not want to ship.

1. The per-worker Mercure hub is dropped

assignMercureHub() fills workerConfig.options, but the new toWorkerOptions() builds a fresh slice and never appends it, where Start() used to pass w.options directly. The field is now written and never read.

WithWorkerMercureHub() appends WithMercureHub() to the worker request options, which is what lets a worker publish from its startup code, outside frankenphp_handle_request(). Request-scoped publishing kept working because the module carries WithMercureHub() in its own request options, which is why no test noticed. #2543 extends the same function and inherits the bug.

2. FrankenPHPModule.server can be nil in ServeHTTP()

It is assigned by FrankenPHPApp.Start(), but caddy starts apps by ranging over a map, so the http app can begin serving first. The handler dereferenced it unconditionally, turning that window into a nil pointer panic where main returned a clean ErrNotRunning.

Two neighbouring lifecycle issues are fixed in the same commit: modules left in app.modules by a config that failed to provision are registered again by the next reload (the app is a process singleton and reset() only runs at the end of Start()), and match in a global worker block was parsed then silently ignored, so it is now rejected at parse time as docs/config.md already documents.

3. A Server could not be registered twice

unregisterServers() only flipped isRegistered, so the worker slices and maps kept the previous run's entries and a second Init() with the same *Server failed with two workers in a server cannot have the same filename. Caddy dodges this by building a fresh NewServer per Start(), but docs/library.md presents NewServer + Init as the library pattern with no hint the instance is single-use.

Same commit: the server_<idx> default was written into s.name permanently, servers were marked registered about a hundred lines before initWorkers() and the thread setup (a request in that window found neither a worker nor a regular thread, so activateServers() is split out), and WithWorkerMatcher() without WithWorkerServerScope() was a silent no-op that still got path-matched via globalWorkersByPath, the opposite of what the matcher asked for.

4. Logger and prepared env

Worker startup contexts hardcoded globalLogger even though the worker's server was at hand, so worker boot messages and worker stdout bypassed the per-php_server logger this branch introduces. The module used to append WithRequestLogger() to each worker's request options and that line is gone, so nothing else covered it.

go_register_server_variables() merged the prepared env whenever the server had one, but registerPreparedEnv() only runs from go_update_request_info(), which returns early without a request. For a server-scoped extension worker handling a message the merge copied whatever the thread-local prepared env held from an earlier request.

Tests

Added: toWorkerOptions() keeps provisioned options, a Server re-registered after Shutdown() still serves its worker and keeps its name, and a request matcher without a server scope is rejected.

Both modules pass go vet including test files. The runtime tests need CI: on my box (WSL2) per-thread engine bootstrap of an embed ZTS build is pathologically slow and linking needs dev libs I do not have, so every Init()-based test is unrunnable locally, including on unmodified base commits.

Not included

worker.mercureHub is assigned by configureMercure() and never read anywhere; removing the dead field touches the nomercure build-tag pair, so I left it alone. Say the word and I will fold it in.

toWorkerOptions() builds a fresh option slice and never appends
workerConfig.options, which assignMercureHub() fills with
WithWorkerMercureHub(). Start() used to pass w.options directly, so the
per-worker Mercure hub is dropped: the field is now written and never
read.

WithWorkerMercureHub() also appends WithMercureHub() to the worker
request options, which is what lets a worker publish from its startup
code, outside frankenphp_handle_request(). Request-scoped publishing kept
working because the module carries WithMercureHub() in its own request
options, which is why no test noticed.
Three lifecycle issues around FrankenPHPApp:

- FrankenPHPModule.server is assigned by FrankenPHPApp.Start(), but caddy
  starts apps by ranging over a map, so the http app can begin serving
  first. ServeHTTP() dereferenced it unconditionally, turning that window
  into a nil pointer panic where it used to be a clean ErrNotRunning.
  Same exposure when registerModules() fails partway through.

- Modules append themselves to app.modules during their own Provision(),
  and app.modules is only cleared by reset(), which runs at the end of
  Start(). A config that fails to provision therefore leaves ghost
  modules behind, and since the app is a process singleton they are
  registered again by the next successful reload. Clear the slice when
  the app is provisioned instead of only when it starts.

- "match" in a global worker block was parsed and then silently ignored,
  because a global worker is not attached to a php_server and has no set
  of requests to match against. docs/config.md already documents match as
  php_server only, so reject it at parse time. No existing Caddyfile in
  the test suite uses it. The core-side counterpart is in a later commit.

Also documents that server_idx is set by the Caddyfile adapter and must
not be written by hand, since modules sharing an index share one server.
Server registration:

- unregisterServers() only flipped isRegistered, so s.workers,
  s.workersByPath and s.workersWithRequestMatcher kept the entries added
  by the previous run. Passing the same *Server to Init() again after a
  Shutdown() therefore failed with "two workers in a server cannot have
  the same filename". registerServers() now clears them, and drops the
  dead workerOpts field while there.
- registerServers() also wrote the "server_<idx>" default into s.name, so
  a later registration at another index kept the stale name. The name
  given to NewServer() is kept aside and the default resolved anew.
- servers were marked registered right after the options were parsed,
  about a hundred lines before initWorkers() and the thread setup. A
  request arriving in that window found neither a worker nor a regular
  thread. Splitting activateServers() out of registerServers() closes it.
- servers is now cleared by unregisterServers() and resetGlobals(), like
  every other worker global.

Request matchers:

WithWorkerMatcher() on a worker without WithWorkerServerScope() was
silently a no-op: initWorkers() only reaches addWorker(), which fills
workersWithRequestMatcher, for server-scoped workers, and the fallback
server's list stays empty forever. Worse, the worker still landed in
globalWorkersByPath and got path-matched, the opposite of what the
matcher asked for. newWorker() now rejects the combination, following
the same shape as the existing filename and name checks.

newWorker() also returned a non-nil worker next to the name-collision
error while the filename path returned nil; no caller looked at it.
newWorkerDummyContext() and newContextFromMessage() hardcoded
globalLogger even though the worker's server was at hand, so worker
startup messages and worker stdout bypassed the per-php_server logger
this branch introduces. The module used to append WithRequestLogger() to
each worker's request options and that line is gone, so nothing else
covered it.

go_register_server_variables() merged the prepared env whenever the
server had one, but registerPreparedEnv() only runs from
go_update_request_info(), which returns early without a request. For a
server-scoped extension worker handling a message the merge therefore
copied whatever the thread-local prepared env held from an earlier
request. Both now agree on the same condition.
@henderkes
henderkes requested a review from AlliBalliBaba July 28, 2026 16:51
Comment thread caddy/module.go

// ServeHTTP implements caddyhttp.MiddlewareHandler.
func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ caddyhttp.Handler) error {
// the server is assigned in FrankenPHPApp.Start(), which caddy may run after the http app started serving

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that order is possible. Everything starts first before accepting requests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants