Define domain specific workers in `php_server` and `php` blocks by henderkes · Pull Request #1509 · php/frankenphp

henderkes · 2025-04-18T05:36:03Z

This enables:

{
frankenphp
}
one.example.com {
 root/var/www/app/public/ php_server {
 root/var/www/app/public/ # the root directive above is not possible to obtain inside this block, I think? env APP_ENV one
 workerindex.php 16
}
}
two.example.com {
 root/var/www/app/public/ # same path as one.example.com! php_server {
 root/var/www/app/public/ env APP_ENV two
 worker {
 fileindex.php num 8
}
}
}

This is equivalent to:

{
frankenphp {
worker {
file /var/www/app/public/index.php
num 16
env APP_ENV one
}
worker {
file /var/www/symlinktoapp/public/index.php
num 8
env APP_ENV two
}
}
one.example.com {
 root/var/www/app/public/ php_server {
 root/var/www/app/public/ env APP_ENV one
}
}
two.example.com {
 root/var/www/symlinktoapp/public/ php_server {
 root/var/www/symlinktoapp/public/ env APP_ENV two
}
}

But we don't need to define symlinks anymore!

Adds the ability to define workers in a domain that inherit their parent blocks environment variables and absolute path.

Surprisingly, my lack of Go knowledge didn't hurt as much as my lack of Caddy internals knowledge. As a consequence, there are many things that I'm very unhappy with. I'll list those in a review...

henderkes · 2025-04-18T05:37:22Z

+// moduleWorkers is a package-level variable to store workers that can be accessed by both FrankenPHPModule and FrankenPHPApp
+var moduleWorkers []workerConfig


Yes, I absolutely hate this, but I haven't found another way to share information between FrankenPHPApp, where the workers live, and FrankenPHPModule.

Yeah I'm not sure if there is a way to do this 'properly' with Caddy. The general issue stems from the fact that PHP has to be started as a singular global instance in the process, so we're probably not getting around some amount of globals anyways.
I think this is fine for now. In the future it might even make sense to refactor this into a global struct to make it possible to optionally omit the frankenphp directive in the Caddy configuration

…on, can't rely on them staying the same

henderkes · 2025-04-18T07:58:49Z

@AlliBalliBaba Let me know what you think about the pain points I've mentioned here.

AlliBalliBaba · 2025-04-19T09:56:52Z

+// moduleWorkers is a package-level variable to store workers that can be accessed by both FrankenPHPModule and FrankenPHPApp
+var moduleWorkers []workerConfig


Yeah I'm not sure if there is a way to do this 'properly' with Caddy. The general issue stems from the fact that PHP has to be started as a singular global instance in the process, so we're probably not getting around some amount of globals anyways.
I think this is fine for now. In the future it might even make sense to refactor this into a global struct to make it possible to optionally omit the frankenphp directive in the Caddy configuration

henderkes · 2025-04-19T12:22:53Z

#1509 (comment)

It might also be possible to use the worker name instead of the ID. We just would have to ensure worker names are unique.

I can't comment directly under it, so here: that would make specifying a worker name mandatory, wouldn't it? That's why I don't particularly like it.

henderkes · 2025-04-19T12:29:04Z

#1509 (comment)

I think this is fine for now. In the future it might even make sense to refactor this into a global struct to make it possible to optionally omit the frankenphp directive in the Caddy configuration

Actually a good idea, by the time Provision is called we already have the information if any of our stuff is called or not. I'll refactor this out into a global struct instance so we can make it optional easier in a future PR.

… newWorker with a filepath that already has a suitable worker, simply add number of threads

…iting logic

withinboredom · 2025-04-26T13:46:58Z

 }

 func TestTransitionRegularThreadToWorkerThread(t *testing.T) {
+	workers = nil


fyi; this doesn't actually stop the workers (assuming they aren't stopped) -- it just loses the references to them. In other words, it may mask test failures in other tests.

Isn't go a garbage collected language and would automatically claim back the resources and memory? If not, I don't think it matters all too much since this isn't something that would happen outside of tests, but I'll look into cleanly shutting them down.

This should be fine in this case, worker threads are stopped at the end of the test with drainPHPThreads() (I'll probably refactor this at some point)

henderkes · 2025-04-26T15:04:52Z

Isn't go a garbage collected language and would automatically claim back the resources and memory? If not, I don't think it matters all too much since this isn't something that would happen outside of tests, but I'll look into cleanly shutting them down.

henderkes · 2025-04-22T08:05:00Z

+ // Check if a worker with this name already exists
+ for _, existingWorker := range moduleWorkers {
+ if existingWorker.Name == wc.Name {
+ return fmt.Errorf("workers must not have duplicate names: %s", wc.Name)
+ }
+ }


I'm not 100% sure if we should allow this or not.
Reasons to allow it: automatically named workers, based on environment variables, should absolutely be able to reuse the same name.
one.example.com { php { root/path/to/app/ workerindex.php 2 } } two.example.com { php { root/path/to/app/ workerindex.php 1 } }
Would absolutely work, because they both have the same (no) environment variables.
one.example.com { php { root/path/to/app/ env APP_ENV one worker { name wrk fileindex.php } } } two.example.com { php { root/path/to/app/ env APP_ENV two worker { name wrk fileindex.php } } } # ortwo.example.com { php { root/path/to/app/ worker { name wrk fileindex2.php } } }
Should absolutely not work, because they have different environment variables.

Current behaviour is that both would fail, but I'm not so happy about the first case. It should just create a worker with 3 threads.

henderkes · 2025-04-23T00:50:40Z

+//go:build !nocaddy
+// +build !nocaddy


No, I've added them when creating and debugging the config_tests. I can remove the nocaddy tag.

henderkes · 2025-04-23T00:57:14Z

+	err = frankenphp.WithModuleWorker(workerName)(fc)
+	if err != nil {
+ return caddyhttp.Error(http.StatusInternalServerError, err)
+	}

+	fr, err := frankenphp.NewRequestWithExistingContext(r, fc)


I'll give it a shot! Still very new to the project, so a lot of these design decisions will not be up to standards yet. Thank you for the hint :)

henderkes · 2025-04-23T01:01:48Z

+func WithModuleWorker(name string) RequestOption {
+	return func(o *frankenPHPContext) error {
+ o.workerName = name
+
+ return nil
+	}
+}


I named it "WithWorkerName" so that it wouldn't accidentally be confused with the WithWorkers(workeropts...) function. I also don't think "WithWorker" is a good name, because it would imply we're passing a worker - but we're not!

henderkes · 2025-04-23T01:50:17Z

+	err = frankenphp.WithModuleWorker(workerName)(fc)
+	if err != nil {
+ return caddyhttp.Error(http.StatusInternalServerError, err)
+	}

+	fr, err := frankenphp.NewRequestWithExistingContext(r, fc)


Unfortunately I think it is necessary. root + path doesn't strip out suffixes from the path information, doesn't follow the specified splitpath and may not correctly resolve the root correctly in case of an embedded app. What can be done is to call the existing NewRequestWithContext twice and parse the filename out after the first time... I don't think that makes sense.
Or I factor the request creation logic out like I did and create a new method to pass the already parsed out context, in order not to perform the same logic twice. I chose the latter and I think that's still the better choice. Wasting performance just to save a public function isn't worth it, imo, especially because libraries may have a legitimate need to parse the filename and other information out, too.

henderkes · 2025-04-26T15:03:00Z

 }

 func TestTransitionRegularThreadToWorkerThread(t *testing.T) {
+	workers = nil


Isn't go a garbage collected language and would automatically claim back the resources and memory? If not, I don't think it matters all too much since this isn't something that would happen outside of tests, but I'll look into cleanly shutting them down.

AlliBalliBaba · 2025-04-26T15:36:08Z


+func getWorkerKey(name string, filename string) string {
+	key := filename
+	if strings.HasPrefix(name, "🧩 ") {


Not sure if a unicode character as prefix is a good idea since worker names are also used in metrics.

I took inspiration from the elephant in the logs. Is there something specific about metrics why this wouldn't be okay?

I'm not using Prometheus metrics myself, maybe unicode characters are also fine @withinboredom wdyt?

There are Prometheus tests already -- I'd maybe add a test for this case if it isn't covered already.

Maybe could we use a # or something like that instead. ASCII compatibility is safer.

Yeah I've used m# before, see 4cc8893

IndraGunawan · 2025-04-28T08:07:52Z

i might have lost track of the changes on this PR.

why do we need to add "identifier" to the worker name?

one.example.com {
 php {
 root/path/to/app/ workerindex.php 2
}
}
two.example.com {
 php {
 root/path/to/app/ workerindex.php 1
}
}

will it create 2 different workers or 1 worker with 3 thread?

henderkes · 2025-04-28T16:49:08Z

it will create two different workers: m#/path/to/app/index.php with two threads and m#/path/to/app/index.php2 with one thread. The m# identifier is given to worker threads so that we can use the name in the slice rather than the filename without breaking BC for libraries that use global workers with names but expect the filename to be the key in the map.

Otherwise, I would have just reworked all code to always use the name, rather than the filename.

dunglas

Are we sure that changes in workers config will be taken into account?

henderkes · 2025-04-29T01:29:14Z

Are we sure that changes in workers config will be taken into account?

During runtime with the admin API or when?

dunglas · 2025-04-29T06:35:58Z

Yes, during runtime.

henderkes · 2025-04-29T07:38:13Z

I haven't looked into caddy's source code so I can't say for sure, but the way I've found it explained in the documentation is that the new configuration is parsed, all caddy modules are Provision()'ed and Start()'ed. This would call FrankenPHPApp::Start() again, which will create workers again:

// Add workers from FrankenPHPApp and FrankenPHPModule configurations// f.Workers may have been set by JSON config, so keep them separatefor_, w:=rangeappend(f.Workers, moduleWorkerConfigs...) {
opts=append(opts, frankenphp.WithWorkers(w.Name, repl.ReplaceKnown(w.FileName, ""), w.Num, w.Env, w.Watch))
}
frankenphp.Shutdown()
iferr:=frankenphp.Init(opts...); err!=nil {
returnerr
}

dunglas · 2025-04-29T08:10:12Z

That would be nice to try, just to be sure.

henderkes · 2025-04-29T09:41:13Z

I've added a test to load a module worker config dynamically.

dunglas

Can you give me push access, please @henderkes? I've made some minor changes.

dunglas · 2025-05-05T11:50:23Z


+func getWorkerKey(name string, filename string) string {
+	key := filename
+	if strings.HasPrefix(name, "m#") {


Couldn't we always use the name if the filename is duplicated? This would remove a soft coupling with the Caddy module.

We wanted to error out if the filename of global workers was duplicated, hence we'd still need a prefix check somewhere. I might be mistaken, but I don't see a way around it without infringing on BC in some way.

It's not a BC break, but it's mixing the responsibilities of the standalone library and of the Caddy module. The notion of "module" is Caddy-specific and should leak into the standalone lib.

henderkes · 2025-05-05T12:05:47Z

Oh, I thought you had push access because you're in the static-php organisation. Added you!

dunglas · 2025-05-05T14:14:35Z

Thank you! Great piece of work.

henderkes · 2025-05-05T14:24:33Z

🎉

dunglas · 2025-05-16T12:34:56Z

We should add this new syntax to the docs!

henderkes · 2025-05-16T13:09:13Z

I'll do that later today!

henderkes · 2025-05-17T03:44:38Z

Documentation added to #1571

henderkes commented Apr 18, 2025
View reviewed changes

henderkes force-pushed the workers branch from 093bf15 to 9ec3fc5Compare April 18, 2025 07:53

henderkes added 5 commits April 18, 2025 14:54

add module (php_server directive) based workers
a165c4e

refactor moduleID to uintptr for faster comparisons
61bbef4

let workers inherit environment variables and root from php_server
7fffbc2

caddy can shift FrankenPHPModules in memory for some godforsaken reas…
60c3e12
…on, can't rely on them staying the same

remove debugging statement
97913f8

henderkes force-pushed the workers branch from 9ec3fc5 to 97913f8Compare April 18, 2025 07:54

fix tests
a22cd19

henderkes commented Apr 18, 2025
View reviewed changes

Comment threadcaddy/caddy.go

AlliBalliBaba reviewed Apr 19, 2025
View reviewed changes

refactor moduleID to uint64 for faster comparisons
a4016df

henderkes force-pushed the workers branch from 7dbe6c6 to a4016dfCompare April 19, 2025 14:06

henderkes added 2 commits April 19, 2025 22:06

actually allow multiple workers per script filename
a21d3f4

remove logging
7718a8e

AlliBalliBaba reviewed Apr 19, 2025
View reviewed changes

Comment threadscaling.go Outdated

henderkes force-pushed the workers branch 2 times, most recently from ecb8ef6 to 571ce92Compare April 19, 2025 16:54

utility function
c7172d2

henderkes force-pushed the workers branch 3 times, most recently from 1d015bf to c7172d2Compare April 19, 2025 17:17

henderkes added 2 commits April 20, 2025 00:32

reuse existing worker with same filename and environment when calling…
f955187
… newWorker with a filepath that already has a suitable worker, simply add number of threads

no cleanup happens between tests, so restore old global worker overwr…
e362fd3
…iting logic

henderkes commented Apr 19, 2025
View reviewed changes

Comment threadworker.go

henderkes commented Apr 19, 2025
View reviewed changes

Comment threadscaling.go Outdated

AlliBalliBaba reviewed Apr 19, 2025
View reviewed changes

Comment threadscaling.go Outdated
Comment threadworker.go
Comment threadworker.go
Comment threadworker.go Outdated

revert1c414ce
3f8f5ec

withinboredom reviewed Apr 26, 2025
View reviewed changes

henderkes commented Apr 26, 2025
View reviewed changes

AlliBalliBaba reviewed Apr 26, 2025
View reviewed changes

revert4cc8893
a6596c7

dunglas reviewed Apr 28, 2025
View reviewed changes

Comment threadcaddy/caddy.go Outdated
Comment threadcaddy/caddy.go Outdated
Comment threadcaddy/caddy.go Outdated
Comment threadcaddy/caddy.go Outdated

apply suggestions
d5d2eb3

henderkes added 2 commits April 29, 2025 16:37

add dynamic config loading test of module worker
6f27895

Merge remote-tracking branch 'dunglas/main' into workers
a6b840f

henderkesand others added 3 commits April 29, 2025 16:53

fix test
877a6ce

Merge branch 'main' into workers
94bce80

minor changes
73d3266

dunglas reviewed May 5, 2025
View reviewed changes

Merge branch 'main' into workers
1c38f0d

dunglas merged commit 1d74b2c into php:mainMay 5, 2025

AlliBalliBaba mentioned this pull request May 11, 2025
feat: optimized worker directives (php_worker) #1552
Closed

		// moduleWorkers is a package-level variable to store workers that can be accessed by both FrankenPHPModule and FrankenPHPApp
		var moduleWorkers []workerConfig

Uh oh!

Conversation

henderkes commented Apr 18, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

henderkes commented Apr 18, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

henderkes commented Apr 19, 2025

Uh oh!

henderkes commented Apr 19, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henderkes commented Apr 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henderkesApr 26, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IndraGunawan commented Apr 28, 2025

Uh oh!

henderkes commented Apr 28, 2025

Uh oh!

dunglas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

henderkes commented Apr 18, 2025•
edited
Loading

henderkesApr 26, 2025•
edited
Loading