Define domain specific workers in `php_server` and `php` blocks #1509

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
        worker index.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 {
            file index.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

caddy/caddy.go

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

caddy/caddy.go

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

caddy/caddy.go

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

caddy/caddy.go

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

caddy/caddy.go

request_options.go

frankenphp.go

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.

scaling.go

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

…iting logic

worker.go

scaling.go

worker.go

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

phpmainthread_test.go

@@ -32,6 +32,7 @@ func TestStartAndStopTheMainThreadWithOneInactiveThread(t *testing.T) {
 }

 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

caddy/caddy.go

+				// 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/ worker index.php 2 } } two.example.com { php { root /path/to/app/ worker index.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 file index.php } } } two.example.com { php { root /path/to/app/ env APP_ENV two worker { name wrk file index.php } } } # or two.example.com { php { root /path/to/app/ worker { name wrk file index2.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

caddy/caddy_test.go

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


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

caddy/caddy.go

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

caddy/caddy.go

+	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

request_options.go

+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

caddy/caddy.go

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

scaling.go

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

phpmainthread_test.go

@@ -32,6 +32,7 @@ func TestStartAndStopTheMainThreadWithOneInactiveThread(t *testing.T) {
 }

 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

worker.go

@@ -64,12 +67,25 @@ func initWorkers(opt []workerOpt) error {
 	return nil
 }

+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/
        worker index.php 2
    }
}
two.example.com {
    php {
        root /path/to/app/
        worker index.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?

caddy/caddy.go

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 separate
for _, w := range append(f.Workers, moduleWorkerConfigs...) {
	opts = append(opts, frankenphp.WithWorkers(w.Name, repl.ReplaceKnown(w.FileName, ""), w.Num, w.Env, w.Watch))
}

frankenphp.Shutdown()
if err := frankenphp.Init(opts...); err != nil {
	return err
}

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

worker.go

@@ -64,12 +65,30 @@ func initWorkers(opt []workerOpt) error {
 	return nil
 }

+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 9ec3fc5 Compare 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 97913f8 Compare April 18, 2025 07:54

fix tests

a22cd19

henderkes commented Apr 18, 2025

View reviewed changes

caddy/caddy.go Show resolved Hide resolved

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 a4016df Compare 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

scaling.go Outdated Show resolved Hide resolved

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

utility function

c7172d2

henderkes force-pushed the workers branch 3 times, most recently from 1d015bf to c7172d2 Compare 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

worker.go Show resolved Hide resolved

henderkes commented Apr 19, 2025

View reviewed changes

scaling.go Outdated Show resolved Hide resolved

AlliBalliBaba reviewed Apr 19, 2025

View reviewed changes

scaling.go Outdated Show resolved Hide resolved

worker.go Show resolved Hide resolved

worker.go Show resolved Hide resolved

worker.go Outdated Show resolved Hide resolved

revert 1c414ce

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

revert 4cc8893

a6596c7

dunglas reviewed Apr 28, 2025

View reviewed changes

caddy/caddy.go Outdated Show resolved Hide resolved

caddy/caddy.go Outdated Show resolved Hide resolved

caddy/caddy.go Outdated Show resolved Hide resolved

caddy/caddy.go Outdated Show resolved Hide resolved

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

henderkes and 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:main May 5, 2025
42 checks passed

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

Define domain specific workers in php_server and php blocks #1509

Define domain specific workers in php_server and php blocks #1509

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!

henderkes Apr 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!

Define domain specific workers in `php_server` and `php` blocks #1509

Define domain specific workers in `php_server` and `php` blocks #1509

henderkes commented Apr 18, 2025 •

edited

Loading

henderkes Apr 26, 2025 •

edited

Loading