feat: unify php_server logic#2499
Conversation
AlliBalliBaba
force-pushed
the
refactor/phpserver
branch
from
09cd397 to
39db699
Compare
|
The public api is now minimized to these 4 functions: // server registration
func WithServer(idx int, resolvedDocumentRoot string,splitPath []string,env map[string]string) Option
// worker scoping
func WithWorkerServerScope(serverIdx int) WorkerOption
func WithWorkerMatcher(matcherFunc func(*http.Request) bool) WorkerOption
// request scoping, less overhead than WithWorkerRequestScope()
func ServeHTTPSrv(serverIdx int, responseWriter http.ResponseWriter, request *http.Request, opts ...RequestOption) errorThis PR also removes the circular fc -> request dependency, which allows simplifying the contextHolder and saving some overhead. |
AlliBalliBaba
changed the title
|
Why do we need the index to be explicitly passed? Couldn't we generate it and return it if needed? |
|
Same reason as in #2487. The caddy modules might be converted to and from JSON multiple times between parsing, provisioning and the actual server start. So the index must already be present when parsing the Caddyfile. Not sure how to make it more elegant, maybe somehow like this? type ServerIdx int |
|
Hum ok got it. I indeed prefer having a custom opaque type (could be an unit under the hood btw) |
|
Alright I changed it so type Server struct { idx }
func WithServer(idx int, resolvedDocumentRoot string,splitPath []string,env map[string]string) (Server, Option)
func WithWorkerServerScope(Server) WorkerOption
func Server.ServeHTTP(responseWriter http.ResponseWriter, request *http.Request, opts ...RequestOption) errorI also noticed that currently configuration would bleed across tests, which probably caused some flakiness. I fixed it by purging configuration right after Init(). Also noticed that When refactoring the context I also fixed some inconsistencies with which logger was being used and which original request method was passed in the debug state. |
| if mainThread.maxThreads <= mainThread.numThreads { | ||
| return | ||
| } | ||
|
|
||
| // reused across reloads so queued requests aren't orphaned on a stale channel | ||
| if scaleChan == nil { | ||
| scaleChan = make(chan *frankenPHPContext) | ||
| } | ||
|
|
||
| if mainThread.maxThreads <= mainThread.numThreads { | ||
| return | ||
| } | ||
|
|
||
| done := mainThread.done |
There was a problem hiding this comment.
fyi @henderkes I realized we can just order it like this and there won't be any race or performance degradation across reloads (#2470).
Makes the FrankenPHP Hello World only lag 30% behind the Caddy Hello World in an ideal setup for me.
There was a problem hiding this comment.
Good catch, no need for the channel if scaling is disabled. I'll give this PR a proper review soon,
There was a problem hiding this comment.
Found some issues regarding failed startup I believe. I also need to take another look at the registration logic. Bit too late now, so tomorrow.
There was a problem hiding this comment.
If you mean the app not resetting on failure, then good catch, should be fixed.
Take your time, this ended up being tons of refactored lines
|
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) |
| f.Workers[i] = wc | ||
| } | ||
| // module worker names are prefixed with m<idx>#, to distinguish them from global workers | ||
| serverPrefix := fmt.Sprintf("m%d#", f.ServerIdx) |
There was a problem hiding this comment.
app.go checks those for worker registration too, doesn't seem to be in the changeset there, so that would be a bug
There was a problem hiding this comment.
Are you talking about duplicate name registration? Not sure what you mean
There was a problem hiding this comment.
You've changed the naming scheme from m# to m<idx># but there are still other references to m# to determine if something is a module worker.
There was a problem hiding this comment.
You're right there's still a reference, I'll change it.
m<idx># kind of contains the info about which module, or would you rather keep "m#"? It's not used internally anymore, so it's just for naming convention and to avoid name overlapping.
There was a problem hiding this comment.
I'd also be fine with removing the prefix entirely or changing it to something like php1#filename, php2#filename
| // ignore modules with a duplicate index | ||
| // this can happen if multiple "php" modules are defined within the same caddy subroute | ||
| if existingModule, ok := modulesByIndex[module.ServerIdx]; ok { | ||
| module.server = existingModule.server | ||
| continue | ||
| } |
There was a problem hiding this comment.
Is POSTing an update to a caddy config with an existing index ever a use case @henderkes ? Then maybe we should only register the last incoming module with the index instead of the first one. The ones without index are now registered regardless.
4 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)