From 424ca426cb037b8f69467c14bbed6e142cd6466a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C3=A9vin=20Dunglas?= Date: Tue, 11 Mar 2025 17:34:49 +0100 Subject: [PATCH] fix: timeouts handling on macOS (#1435) * ci: run tests on macOS * debug * debug * fix * nobrotli * install brotli * fix * fix * Also registers php.ini if ZEND_MAX_EXECUTION_TIMERS is disabled. * Removes max_execution_time from tests (it gets overwritten on mac) * tiny refacto * fix free * cs --------- Co-authored-by: Alliballibaba --- .github/workflows/tests.yaml | 39 +++++++++++++++++++++++++++++++++++- CONTRIBUTING.md | 9 +++------ caddy/caddy_test.go | 8 ++++---- frankenphp.c | 36 ++++++++++----------------------- phpmainthread.go | 30 ++++++++++++++++++--------- 5 files changed, 77 insertions(+), 45 deletions(-) diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 4efb0693..f4ae6b88 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -14,7 +14,8 @@ on: permissions: contents: read jobs: - tests: + tests-linux: + name: Tests (Linux, PHP ${{ matrix.php-versions }}) runs-on: ubuntu-latest strategy: fail-fast: false @@ -71,3 +72,39 @@ jobs: if: matrix.php-versions == '8.4' with: version: latest + tests-mac: + name: Tests (macOS, PHP 8.4) + runs-on: macos-latest + env: + GOEXPERIMENT: cgocheck2 + HOMEBREW_NO_AUTO_UPDATE: 1 + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-go@v5 + with: + go-version: "1.24" + cache-dependency-path: | + go.sum + caddy/go.sum + - uses: shivammathur/setup-php@v2 + with: + php-version: 8.4 + ini-file: development + coverage: none + tools: none + env: + phpts: ts + debug: true + - name: Set Set CGO flags + run: | + { + echo "CGO_CFLAGS=-I/opt/homebrew/include/ $(php-config --includes)" + echo "CGO_LDFLAGS=-L/opt/homebrew/lib/ $(php-config --ldflags) $(php-config --libs)" + } >> "${GITHUB_ENV}" + - name: Build + run: go build -tags nowatcher + - name: Run library tests + run: go test -tags nowatcher -race -v ./... + - name: Run Caddy module tests + working-directory: caddy/ + run: go test -tags nowatcher,nobadger,nomysql,nopgx -race -v ./... diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bf5c5ebd..b7b7796e 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -149,16 +149,13 @@ docker buildx bake -f docker-bake.hcl --pull --no-cache --push 3. Enable `tmate` to connect to the container ```patch - - - name: Set CGO flags + - name: Set CGO flags run: echo "CGO_CFLAGS=$(php-config --includes)" >> "$GITHUB_ENV" - + - - + run: | + + - run: | + sudo apt install gdb + mkdir -p /home/runner/.config/gdb/ + printf "set auto-load safe-path /\nhandle SIG34 nostop noprint pass" > /home/runner/.config/gdb/gdbinit - + - - + uses: mxschmitt/action-tmate@v3 + + - uses: mxschmitt/action-tmate@v3 ``` 4. Connect to the container diff --git a/caddy/caddy_test.go b/caddy/caddy_test.go index 0d374f47..2ff8bf20 100644 --- a/caddy/caddy_test.go +++ b/caddy/caddy_test.go @@ -660,7 +660,7 @@ func TestPHPIniConfiguration(t *testing.T) { frankenphp { num_threads 2 worker ../testdata/ini.php 1 - php_ini max_execution_time 100 + php_ini upload_max_filesize 100M php_ini memory_limit 10000000 } } @@ -673,7 +673,7 @@ func TestPHPIniConfiguration(t *testing.T) { } `, "caddyfile") - testSingleIniConfiguration(tester, "max_execution_time", "100") + testSingleIniConfiguration(tester, "upload_max_filesize", "100M") testSingleIniConfiguration(tester, "memory_limit", "10000000") } @@ -688,7 +688,7 @@ func TestPHPIniBlockConfiguration(t *testing.T) { frankenphp { num_threads 1 php_ini { - max_execution_time 15 + upload_max_filesize 100M memory_limit 20000000 } } @@ -702,7 +702,7 @@ func TestPHPIniBlockConfiguration(t *testing.T) { } `, "caddyfile") - testSingleIniConfiguration(tester, "max_execution_time", "15") + testSingleIniConfiguration(tester, "upload_max_filesize", "100M") testSingleIniConfiguration(tester, "memory_limit", "20000000") } diff --git a/frankenphp.c b/frankenphp.c index c4af6c2a..a20d4018 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -33,13 +33,6 @@ ZEND_TSRMLS_CACHE_DEFINE() #endif -/* Timeouts are currently fundamentally broken with ZTS except on Linux and - * FreeBSD: https://bugs.php.net/bug.php?id=79464 */ -#ifndef ZEND_MAX_EXECUTION_TIMERS -static const char HARDCODED_INI[] = "max_execution_time=0\n" - "max_input_time=-1\n\0"; -#endif - static const char *MODULES_TO_RELOAD[] = {"filter", "session", NULL}; frankenphp_version frankenphp_get_version() { @@ -900,25 +893,18 @@ static void *php_main(void *arg) { sapi_startup(&frankenphp_sapi_module); -#ifndef ZEND_MAX_EXECUTION_TIMERS -#if (PHP_VERSION_ID >= 80300) - frankenphp_sapi_module.ini_entries = HARDCODED_INI; -#else - frankenphp_sapi_module.ini_entries = malloc(sizeof(HARDCODED_INI)); - if (frankenphp_sapi_module.ini_entries == NULL) { - perror("malloc failed"); - exit(EXIT_FAILURE); - } - memcpy(frankenphp_sapi_module.ini_entries, HARDCODED_INI, - sizeof(HARDCODED_INI)); -#endif -#else +#ifdef ZEND_MAX_EXECUTION_TIMERS /* overwrite php.ini with custom user settings */ - char *php_ini_overrides = go_get_custom_php_ini(); + char *php_ini_overrides = go_get_custom_php_ini(false); +#else + /* overwrite php.ini with custom user settings and disable + * max_execution_timers */ + char *php_ini_overrides = go_get_custom_php_ini(true); +#endif + if (php_ini_overrides != NULL) { frankenphp_sapi_module.ini_entries = php_ini_overrides; } -#endif frankenphp_sapi_module.startup(&frankenphp_sapi_module); @@ -938,13 +924,13 @@ static void *php_main(void *arg) { tsrm_shutdown(); #endif -#if (PHP_VERSION_ID < 80300) if (frankenphp_sapi_module.ini_entries) { - free(frankenphp_sapi_module.ini_entries); + free((char *)frankenphp_sapi_module.ini_entries); frankenphp_sapi_module.ini_entries = NULL; } -#endif + go_frankenphp_shutdown_main_thread(); + return NULL; } diff --git a/phpmainthread.go b/phpmainthread.go index 559c5896..7245db37 100644 --- a/phpmainthread.go +++ b/phpmainthread.go @@ -8,7 +8,7 @@ package frankenphp // #include "frankenphp.h" import "C" import ( - "fmt" + "strings" "sync" "github.com/dunglas/frankenphp/internal/memory" @@ -191,16 +191,28 @@ func go_frankenphp_shutdown_main_thread() { } //export go_get_custom_php_ini -func go_get_custom_php_ini() *C.char { +func go_get_custom_php_ini(disableTimeouts C.bool) *C.char { if mainThread.phpIni == nil { - return nil + mainThread.phpIni = make(map[string]string) } - // pass the php.ini overrides to PHP before startup - // TODO: if needed this would also be possible on a per-thread basis - overrides := "" - for k, v := range mainThread.phpIni { - overrides += fmt.Sprintf("%s=%s\n", k, v) + // Timeouts are currently fundamentally broken + // with ZTS except on Linux and FreeBSD: https://bugs.php.net/bug.php?id=79464 + // Disable timeouts if ZEND_MAX_EXECUTION_TIMERS is not supported + if disableTimeouts { + mainThread.phpIni["max_execution_time"] = "0" + mainThread.phpIni["max_input_time"] = "-1" } - return C.CString(overrides) + + // Pass the php.ini overrides to PHP before startup + // TODO: if needed this would also be possible on a per-thread basis + var overrides strings.Builder + for k, v := range mainThread.phpIni { + overrides.WriteString(k) + overrides.WriteByte('=') + overrides.WriteString(v) + overrides.WriteByte('\n') + } + + return C.CString(overrides.String()) }