Skip to content

feat: unify php_server logic#2499

Open
AlliBalliBaba wants to merge 60 commits into
mainfrom
refactor/phpserver
Open

feat: unify php_server logic#2499
AlliBalliBaba wants to merge 60 commits into
mainfrom
refactor/phpserver

Conversation

@AlliBalliBaba

Copy link
Copy Markdown
Contributor

Currently the concept of a php_server only 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_server blocks.

This PR is an attempt at refactoring the current php_server logic so it is properly mirrored on the FrankenPHP side without BC breaks for library users (and to prevent future bugs like mentioned in #2487)

Comment thread options.go Outdated
Comment thread options.go Outdated
Comment thread options.go Outdated
Comment thread phpserver.go Outdated
Comment thread scaling.go
@AlliBalliBaba

Copy link
Copy Markdown
Contributor Author

I changed the api again a bit, this feels like it makes the most sense (no idx passing required):

server := frankenphp.NewServer(root, env, splitPath)

frankenphp.Init(frankenphp.WithServer(server))

server.ServeHTTP(request, responseWriter)

Comment thread context.go
Comment thread caddy/module.go Outdated
Comment thread caddy/module.go Outdated
Comment thread caddy/app.go Outdated
Comment thread server.go Outdated
Comment thread caddy/app.go Outdated
@AlliBalliBaba

Copy link
Copy Markdown
Contributor Author

Another subtlety I fixed: the "external workers" from RegisterWorkers should probably persist across reloads.

@AlliBalliBaba AlliBalliBaba requested a review from henderkes July 12, 2026 22:23

@henderkes henderkes left a comment

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 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).

Comment thread server.go
@AlliBalliBaba

Copy link
Copy Markdown
Contributor Author

DrainWorkers always worked this way. I think in the past it was a way for library users to have a graceful worker shutdown. But it is not necessary anymore. IMO we can also remove it

@henderkes

Copy link
Copy Markdown
Contributor

+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.

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.

4 participants