fix: keep custom extensions registered after worker reload by ousamabenyounes · Pull Request #2579 · php/frankenphp

ousamabenyounes · 2026-07-29T10:28:24Z

Summary

Custom FrankenPHP extensions registered through RegisterExtension() were only available during the first PHP engine startup. Worker restarts and watch-triggered hot reloads reboot the PHP main thread, which runs php_module_startup() again, but the custom module list had already been consumed.

This keeps the registered module list available for every PHP startup in the current process and stores a C-owned copy of the module-entry pointer array so the hook does not point at Go slice storage after registration.

Fix #2572

Test verification (RED -> GREEN)

RED on origin/main with only the regression test added:

=== RUN   TestRegisterExtension
2026/07/29 10:01:34 INFO FrankenPHP started php_version=8.5.10-dev num_threads=16 max_threads=16 max_requests=0
2026/07/29 10:01:34 INFO rebooting all PHP threads
2026/07/29 10:01:34 INFO thread reboot finished duration=24.272685ms num_threads=16
Error: "Array ... [29] => frankenphp ..." does not contain "ext1"
Error: "Array ... [29] => frankenphp ..." does not contain "ext2"
--- FAIL: TestRegisterExtension (0.10s)
FAIL
FAIL github.com/dunglas/frankenphp/internal/testext 0.122s

GREEN with this patch:

=== RUN   TestRegisterExtension
2026/07/29 10:03:13 INFO FrankenPHP started php_version=8.5.10-dev num_threads=16 max_threads=16 max_requests=0
2026/07/29 10:03:13 INFO rebooting all PHP threads
2026/07/29 10:03:13 INFO thread reboot finished duration=26.36064ms num_threads=16
--- PASS: TestRegisterExtension (0.06s)
PASS
ok github.com/dunglas/frankenphp/internal/testext 0.073s

Full local validation in the FrankenPHP dev Docker environment:

ok github.com/dunglas/frankenphp 17.540s
ok github.com/dunglas/frankenphp/internal/extgen 0.280s
ok github.com/dunglas/frankenphp/internal/phpheaders 0.006s
ok github.com/dunglas/frankenphp/internal/state 0.042s
ok github.com/dunglas/frankenphp/internal/testext 0.187s
ok github.com/dunglas/frankenphp/internal/watcher 0.052s

alexandre-daubois

Approach is right. I checked it against php-src: module_destructor resets module_started and frees the TSRM globals id, zend_startup recreates module_registry on every php_module_startup(), and php_register_internal_extensions_func is not restored at shutdown. The C copy also fixes a real cgo pointer-rule violation, the old code kept &extensions[0] (Go memory) in C after extensions = nil.

Three things I would block on: register_extensions is not idempotent (hook recurses into itself), a reboot-time registration failure is unobservable, and the regression test can pass without ever rebooting. Details inline.

alexandre-daubois · 2026-07-30T09:38:48Z


  original_php_register_internal_extensions_func =
      php_register_internal_extensions_func;
  php_register_internal_extensions_func = register_internal_extensions;


Not idempotent. A second call sets original_php_register_internal_extensions_func = register_internal_extensions, so the hook calls itself and the next php_module_startup() recurses until the main PHP thread blows its stack. It also leaks the previous persistent_modules, and the success path never frees it either.

Only the Go-side sync.Once prevents that today. Now that this function owns permanent process state, the guard belongs here:

if (original_php_register_internal_extensions_func != NULL) { return SUCCESS; }

alexandre-daubois · 2026-07-30T09:38:48Z

-  modules = m;
+int register_extensions(zend_module_entry **m, int len) {
+  zend_module_entry **persistent_modules =
+      malloc(sizeof(zend_module_entry *) * len);


len == 0 makes this implementation-defined: malloc(0) may return NULL, which you report as FAILURE. An if (len <= 0) return SUCCESS; up front drops the dependency on the Go caller's guard.

alexandre-daubois · 2026-07-30T09:38:48Z

-  modules = NULL;
-  modules_len = 0;
-
  return SUCCESS;


Dropping the modules = NULL; modules_len = 0; reset means this loop now runs on every php_module_startup(), so it can return FAILURE on a reload. Nothing acts on it: php_main ignores the result of frankenphp_sapi_module.startup() (L1702) and calls go_frankenphp_main_thread_is_ready() anyway (L1711). rebootAllThreads then flips every thread back to Ready and requests get served on a half-started engine, with no log line.

Before this PR the hook was a no-op after the first startup, so this could only bite at boot. It is now reachable on every watcher-triggered reload.

alexandre-daubois · 2026-07-30T09:38:49Z

+			registerErr = ErrExtensionRegistration
+			return
+		}
 		extensions = nil


After this, registerOnce is consumed and extensions is nil, so any later RegisterExtension() appends to a slice nobody reads again and registerExtensions() still returns nil.

Init(), Shutdown(), RegisterExtension(...), Init(): the second Init() succeeds and the extension is simply absent from get_loaded_extensions(). That is the one failure mode a user can actually hit, and it is silent.

alexandre-daubois · 2026-07-30T09:38:49Z

+	registerErr  error
+
+	// ErrExtensionRegistration is returned when PHP extension module entries cannot be registered.
+	ErrExtensionRegistration = errors.New("error registering PHP extensions")


The only way register_extensions returns FAILURE is malloc(8 * len) failing for a single-digit len. Not reachable in practice, and untestable without fault injection. That buys a permanent exported symbol plus registerErr, cliFailureExitCode and a divergent error path in Init, for a case that cannot happen.

Drop it and report the reachable misuse (RegisterExtension after registration) instead.

alexandre-daubois · 2026-07-30T09:38:49Z

 func ExecuteScriptCLI(script string, args []string) int {
 	// Ensure extensions are registered before CLI execution
-	registerExtensions()
+	if registerExtensions() != nil {


Error thrown away, so this exits 1 with no output at all: caddy/php-cli.go:44 feeds it straight into os.Exit(status). if err := registerExtensions(); err != nil at least lets you log why. Same at L28.

alexandre-daubois · 2026-07-30T09:38:49Z


-	registerExtensions()
+	if err := registerExtensions(); err != nil {
+		isRunning = false


Every other failure in Init calls Shutdown(), which is already safe here (drainPHPThreads returns early when mainThread == nil) and also runs resetGlobals(). No reason for this path to hand-roll it.

alexandre-daubois · 2026-07-30T09:38:49Z


+	assertRegisteredExtensions(t)
+
+	frankenphp.RestartWorkers()


This can be a no-op and the test still passes. RestartWorkers() returns nothing and swallows the rebootAllThreads() bool: it bails when mainThread == nil, and returns false without rebooting when isRebooting is already set or the main thread is not state.Ready. In those cases the second assertRegisteredExtensions(t) is just a copy of the first and stays green, including on a regression of this exact bug. It needs something observable from the reboot.

Related: module1_entry / module2_entry in extensions.c have NULL MINIT/MSHUTDOWN, no function table and no module globals, so this only proves a stateless entry can be re-registered. The risk this PR takes on is php-src re-running the module lifecycle (ts_free_id then ts_allocate_fast_id, MINIT again). Give one of the two modules a MINIT plus module globals and assert its constant is still usable after the restart.

alexandre-daubois · 2026-07-30T09:38:49Z


-	req, err = frankenphp.NewRequestWithContext(req, frankenphp.WithRequestDocumentRoot("./testdata", false))
+	req, err := frankenphp.NewRequestWithContext(req, frankenphp.WithRequestDocumentRoot("./testdata", false))
 	assert.NoError(t, err)


require, not assert: on error req is nil and ServeHTTP dereferences it, so the test panics on a nil pointer instead of reporting a readable failure.

alexandre-daubois · 2026-07-30T09:38:49Z

 )

+const (
+	firstExtensionName  = "ext1"


Each used once, and they no longer match the module1_entry / module2_entry names in extensions.c. Inlining "ext1" / "ext2" reads better.

fix: keep custom extensions registered after worker reload

701d72c

alexandre-daubois reviewed Jul 30, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: keep custom extensions registered after worker reload - #2579

fix: keep custom extensions registered after worker reload#2579
ousamabenyounes wants to merge 1 commit into
php:mainfrom
ousamabenyounes:fix/issue-2572

ousamabenyounes commented Jul 29, 2026

Uh oh!

alexandre-daubois left a comment

Uh oh!

alexandre-daubois Jul 30, 2026

Uh oh!

alexandre-daubois Jul 30, 2026

Uh oh!

alexandre-daubois Jul 30, 2026

Uh oh!

alexandre-daubois Jul 30, 2026

Uh oh!

alexandre-daubois Jul 30, 2026

Uh oh!

alexandre-daubois Jul 30, 2026

Uh oh!

alexandre-daubois Jul 30, 2026

Uh oh!

alexandre-daubois Jul 30, 2026

Uh oh!

alexandre-daubois Jul 30, 2026

Uh oh!

alexandre-daubois Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

ousamabenyounes commented Jul 29, 2026

Summary

Test verification (RED -> GREEN)

Uh oh!

alexandre-daubois left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants