From ca19712f87441656b913f99bf65fd719801ff2aa Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Sat, 1 Nov 2025 21:33:33 +0100 Subject: [PATCH 01/31] Cleanup. --- backoff.go | 51 ---------------------------- backoff_test.go | 41 ---------------------- cgi.go | 6 ++-- env.go | 5 ++- frankenphp.c | 14 ++++---- frankenphp.h | 10 +----- internal/backoff/backoff.go | 58 ++++++++++++++++++++++++++++++++ internal/backoff/backoff_test.go | 41 ++++++++++++++++++++++ phpthread.go | 4 +++ threadregular.go | 1 + threadworker.go | 23 +++++++------ 11 files changed, 128 insertions(+), 126 deletions(-) delete mode 100644 backoff.go delete mode 100644 backoff_test.go create mode 100644 internal/backoff/backoff.go create mode 100644 internal/backoff/backoff_test.go diff --git a/backoff.go b/backoff.go deleted file mode 100644 index a4bce80fa4..0000000000 --- a/backoff.go +++ /dev/null @@ -1,51 +0,0 @@ -package frankenphp - -import ( - "sync" - "time" -) - -type exponentialBackoff struct { - backoff time.Duration - failureCount int - mu sync.RWMutex - maxBackoff time.Duration - minBackoff time.Duration - maxConsecutiveFailures int -} - -// recordSuccess resets the backoff and failureCount -func (e *exponentialBackoff) recordSuccess() { - e.mu.Lock() - e.failureCount = 0 - e.backoff = e.minBackoff - e.mu.Unlock() -} - -// recordFailure increments the failure count and increases the backoff, it returns true if maxConsecutiveFailures has been reached -func (e *exponentialBackoff) recordFailure() bool { - e.mu.Lock() - e.failureCount += 1 - if e.backoff < e.minBackoff { - e.backoff = e.minBackoff - } - - e.backoff = min(e.backoff*2, e.maxBackoff) - - e.mu.Unlock() - return e.maxConsecutiveFailures != -1 && e.failureCount >= e.maxConsecutiveFailures -} - -// wait sleeps for the backoff duration if failureCount is non-zero. -// NOTE: this is not tested and should be kept 'obviously correct' (i.e., simple) -func (e *exponentialBackoff) wait() { - e.mu.RLock() - if e.failureCount == 0 { - e.mu.RUnlock() - - return - } - e.mu.RUnlock() - - time.Sleep(e.backoff) -} diff --git a/backoff_test.go b/backoff_test.go deleted file mode 100644 index 5ced2e4cd5..0000000000 --- a/backoff_test.go +++ /dev/null @@ -1,41 +0,0 @@ -package frankenphp - -import ( - "github.com/stretchr/testify/assert" - "testing" - "time" -) - -func TestExponentialBackoff_Reset(t *testing.T) { - e := &exponentialBackoff{ - maxBackoff: 5 * time.Second, - minBackoff: 500 * time.Millisecond, - maxConsecutiveFailures: 3, - } - - assert.False(t, e.recordFailure()) - assert.False(t, e.recordFailure()) - e.recordSuccess() - - e.mu.RLock() - defer e.mu.RUnlock() - assert.Equal(t, 0, e.failureCount, "expected failureCount to be reset to 0") - assert.Equal(t, e.backoff, e.minBackoff, "expected backoff to be reset to minBackoff") -} - -func TestExponentialBackoff_Trigger(t *testing.T) { - e := &exponentialBackoff{ - maxBackoff: 500 * 3 * time.Millisecond, - minBackoff: 500 * time.Millisecond, - maxConsecutiveFailures: 3, - } - - assert.False(t, e.recordFailure()) - assert.False(t, e.recordFailure()) - assert.True(t, e.recordFailure()) - - e.mu.RLock() - defer e.mu.RUnlock() - assert.Equal(t, e.failureCount, e.maxConsecutiveFailures, "expected failureCount to be maxConsecutiveFailures") - assert.Equal(t, e.backoff, e.maxBackoff, "expected backoff to be maxBackoff") -} diff --git a/cgi.go b/cgi.go index 4c11a285ab..6c36cf6986 100644 --- a/cgi.go +++ b/cgi.go @@ -277,13 +277,13 @@ func splitPos(path string, splitPath []string) int { // See: https://github.com/php/php-src/blob/345e04b619c3bc11ea17ee02cdecad6ae8ce5891/main/SAPI.h#L72 // //export go_update_request_info -func go_update_request_info(threadIndex C.uintptr_t, info *C.sapi_request_info) C.bool { +func go_update_request_info(threadIndex C.uintptr_t, info *C.sapi_request_info) { thread := phpThreads[threadIndex] fc := thread.getRequestContext() request := fc.request if request == nil { - return C.bool(fc.worker != nil) + return } authUser, authPassword, ok := request.BasicAuth() @@ -311,8 +311,6 @@ func go_update_request_info(threadIndex C.uintptr_t, info *C.sapi_request_info) info.request_uri = thread.pinCString(request.URL.RequestURI()) info.proto_num = C.int(request.ProtoMajor*1000 + request.ProtoMinor) - - return C.bool(fc.worker != nil) } // SanitizedPathJoin performs filepath.Join(root, reqPath) that diff --git a/env.go b/env.go index 9e6fbfdfe5..3ac9a3adf6 100644 --- a/env.go +++ b/env.go @@ -1,10 +1,9 @@ package frankenphp // #cgo nocallback frankenphp_init_persistent_string -// #cgo nocallback frankenphp_add_assoc_str_ex // #cgo noescape frankenphp_init_persistent_string -// #cgo noescape frankenphp_add_assoc_str_ex // #include "frankenphp.h" +// #include import "C" import ( "os" @@ -98,7 +97,7 @@ func go_getfullenv(threadIndex C.uintptr_t, trackVarsArray *C.zval) { env := getSandboxedEnv(thread) for key, val := range env { - C.frankenphp_add_assoc_str_ex(trackVarsArray, toUnsafeChar(key), C.size_t(len(key)), val) + C.add_assoc_str_ex(trackVarsArray, toUnsafeChar(key), C.size_t(len(key)), val) } } diff --git a/frankenphp.c b/frankenphp.c index 3d124eced1..99eca7703f 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -51,7 +51,6 @@ frankenphp_version frankenphp_get_version() { frankenphp_config frankenphp_get_config() { return (frankenphp_config){ - frankenphp_get_version(), #ifdef ZTS true, #else @@ -75,6 +74,10 @@ __thread uintptr_t thread_index; __thread bool is_worker_thread = false; __thread zval *os_environment = NULL; +void frankenphp_update_local_thread_context(bool is_worker) { + is_worker_thread = is_worker; +} + static void frankenphp_update_request_context() { /* the server context is stored on the go side, still SG(server_context) needs * to not be NULL */ @@ -82,7 +85,7 @@ static void frankenphp_update_request_context() { /* status It is not reset by zend engine, set it to 200. */ SG(sapi_headers).http_response_code = 200; - is_worker_thread = go_update_request_info(thread_index, &SG(request_info)); + go_update_request_info(thread_index, &SG(request_info)); } static void frankenphp_free_request_context() { @@ -206,11 +209,6 @@ PHPAPI void get_full_env(zval *track_vars_array) { go_getfullenv(thread_index, track_vars_array); } -void frankenphp_add_assoc_str_ex(zval *track_vars_array, char *key, - size_t keylen, zend_string *val) { - add_assoc_str_ex(track_vars_array, key, keylen, val); -} - /* Adapted from php_request_startup() */ static int frankenphp_worker_request_startup() { int retval = SUCCESS; @@ -610,7 +608,7 @@ static char *frankenphp_read_cookies(void) { } /* all variables with well defined keys can safely be registered like this */ -void frankenphp_register_trusted_var(zend_string *z_key, char *value, +static inline void frankenphp_register_trusted_var(zend_string *z_key, char *value, size_t val_len, HashTable *ht) { if (value == NULL) { zval empty; diff --git a/frankenphp.h b/frankenphp.h index c17df6061a..efbd5fc48f 100644 --- a/frankenphp.h +++ b/frankenphp.h @@ -23,12 +23,6 @@ typedef struct ht_key_value_pair { size_t val_len; } ht_key_value_pair; -typedef struct php_variable { - const char *var; - size_t data_len; - char *data; -} php_variable; - typedef struct frankenphp_version { unsigned char major_version; unsigned char minor_version; @@ -40,7 +34,6 @@ typedef struct frankenphp_version { frankenphp_version frankenphp_get_version(); typedef struct frankenphp_config { - frankenphp_version version; bool zts; bool zend_signals; bool zend_max_execution_timers; @@ -52,6 +45,7 @@ bool frankenphp_new_php_thread(uintptr_t thread_index); bool frankenphp_shutdown_dummy_request(void); int frankenphp_execute_script(char *file_name); +void frankenphp_update_local_thread_context(bool is_worker); int frankenphp_execute_script_cli(char *script, int argc, char **argv, bool eval); @@ -65,8 +59,6 @@ void frankenphp_register_variable_safe(char *key, char *var, size_t val_len, zend_string *frankenphp_init_persistent_string(const char *string, size_t len); int frankenphp_reset_opcache(void); int frankenphp_get_current_memory_limit(); -void frankenphp_add_assoc_str_ex(zval *track_vars_array, char *key, - size_t keylen, zend_string *val); void frankenphp_register_single(zend_string *z_key, char *value, size_t val_len, zval *track_vars_array); diff --git a/internal/backoff/backoff.go b/internal/backoff/backoff.go new file mode 100644 index 0000000000..9139623879 --- /dev/null +++ b/internal/backoff/backoff.go @@ -0,0 +1,58 @@ +package backoff + +import ( + "sync" + "time" +) + +type ExponentialBackoff struct { + backoff time.Duration + failureCount int + mu sync.RWMutex + MaxBackoff time.Duration + MinBackoff time.Duration + MaxConsecutiveFailures int +} + +// recordSuccess resets the backoff and failureCount +func (e *ExponentialBackoff) RecordSuccess() { + e.mu.Lock() + e.failureCount = 0 + e.backoff = e.MinBackoff + e.mu.Unlock() +} + +// recordFailure increments the failure count and increases the backoff, it returns true if MaxConsecutiveFailures has been reached +func (e *ExponentialBackoff) RecordFailure() bool { + e.mu.Lock() + e.failureCount += 1 + if e.backoff < e.MinBackoff { + e.backoff = e.MinBackoff + } + + e.backoff = min(e.backoff*2, e.MaxBackoff) + + e.mu.Unlock() + return e.MaxConsecutiveFailures != -1 && e.failureCount >= e.MaxConsecutiveFailures +} + +// wait sleeps for the backoff duration if failureCount is non-zero. +// NOTE: this is not tested and should be kept 'obviously correct' (i.e., simple) +func (e *ExponentialBackoff) Wait() { + e.mu.RLock() + if e.failureCount == 0 { + e.mu.RUnlock() + + return + } + e.mu.RUnlock() + + time.Sleep(e.backoff) +} + +func (e *ExponentialBackoff) FailureCount() int { + e.mu.RLock() + defer e.mu.RUnlock() + + return e.failureCount +} \ No newline at end of file diff --git a/internal/backoff/backoff_test.go b/internal/backoff/backoff_test.go new file mode 100644 index 0000000000..b82efc4bd4 --- /dev/null +++ b/internal/backoff/backoff_test.go @@ -0,0 +1,41 @@ +package backoff + +import ( + "github.com/stretchr/testify/assert" + "testing" + "time" +) + +func TestExponentialBackoff_Reset(t *testing.T) { + e := &ExponentialBackoff{ + MaxBackoff: 5 * time.Second, + MinBackoff: 500 * time.Millisecond, + MaxConsecutiveFailures: 3, + } + + assert.False(t, e.RecordFailure()) + assert.False(t, e.RecordFailure()) + e.RecordSuccess() + + e.mu.RLock() + defer e.mu.RUnlock() + assert.Equal(t, 0, e.failureCount, "expected failureCount to be reset to 0") + assert.Equal(t, e.backoff, e.MinBackoff, "expected backoff to be reset to MinBackoff") +} + +func TestExponentialBackoff_Trigger(t *testing.T) { + e := &ExponentialBackoff{ + MaxBackoff: 500 * 3 * time.Millisecond, + MinBackoff: 500 * time.Millisecond, + MaxConsecutiveFailures: 3, + } + + assert.False(t, e.RecordFailure()) + assert.False(t, e.RecordFailure()) + assert.True(t, e.RecordFailure()) + + e.mu.RLock() + defer e.mu.RUnlock() + assert.Equal(t, e.failureCount, e.MaxConsecutiveFailures, "expected failureCount to be MaxConsecutiveFailures") + assert.Equal(t, e.backoff, e.MaxBackoff, "expected backoff to be MaxBackoff") +} diff --git a/phpthread.go b/phpthread.go index a60aa8f0aa..2691aa54bf 100644 --- a/phpthread.go +++ b/phpthread.go @@ -132,6 +132,10 @@ func (thread *phpThread) pinCString(s string) *C.char { return thread.pinString(s + "\x00") } +func (*phpThread) updateContext(isWorker bool) { + C.frankenphp_update_local_thread_context(C.bool(isWorker)) +} + //export go_frankenphp_before_script_execution func go_frankenphp_before_script_execution(threadIndex C.uintptr_t) *C.char { thread := phpThreads[threadIndex] diff --git a/threadregular.go b/threadregular.go index 88cef7e79d..a8ca03543c 100644 --- a/threadregular.go +++ b/threadregular.go @@ -34,6 +34,7 @@ func (handler *regularThread) beforeScriptExecution() string { detachRegularThread(handler.thread) return handler.thread.transitionToNewHandler() case stateTransitionComplete: + handler.thread.updateContext(false) handler.state.set(stateReady) return handler.waitForRequest() case stateReady: diff --git a/threadworker.go b/threadworker.go index 5a59f9278b..6d3ca6d2ce 100644 --- a/threadworker.go +++ b/threadworker.go @@ -9,6 +9,8 @@ import ( "path/filepath" "time" "unsafe" + + "github.com/dunglas/frankenphp/internal/backoff" ) // representation of a thread assigned to a worker script @@ -20,7 +22,7 @@ type workerThread struct { worker *worker dummyContext *frankenPHPContext workerContext *frankenPHPContext - backoff *exponentialBackoff + backoff *backoff.ExponentialBackoff isBootingScript bool // true if the worker has not reached frankenphp_handle_request yet } @@ -29,10 +31,10 @@ func convertToWorkerThread(thread *phpThread, worker *worker) { state: thread.state, thread: thread, worker: worker, - backoff: &exponentialBackoff{ - maxBackoff: 1 * time.Second, - minBackoff: 100 * time.Millisecond, - maxConsecutiveFailures: worker.maxConsecutiveFailures, + backoff: &backoff.ExponentialBackoff{ + MaxBackoff: 1 * time.Second, + MinBackoff: 100 * time.Millisecond, + MaxConsecutiveFailures: worker.maxConsecutiveFailures, }, }) worker.attachThread(thread) @@ -55,6 +57,7 @@ func (handler *workerThread) beforeScriptExecution() string { handler.state.waitFor(stateReady, stateShuttingDown) return handler.beforeScriptExecution() case stateReady, stateTransitionComplete: + handler.thread.updateContext(true) if handler.worker.onThreadReady != nil { handler.worker.onThreadReady(handler.thread.threadIndex) } @@ -88,7 +91,7 @@ func (handler *workerThread) name() string { } func setupWorkerScript(handler *workerThread, worker *worker) { - handler.backoff.wait() + handler.backoff.Wait() metrics.StartWorker(worker.name) if handler.state.is(stateReady) { @@ -128,7 +131,7 @@ func tearDownWorkerScript(handler *workerThread, exitStatus int) { // on exit status 0 we just run the worker script again if exitStatus == 0 && !handler.isBootingScript { metrics.StopWorker(worker.name, StopReasonRestart) - handler.backoff.recordSuccess() + handler.backoff.RecordSuccess() logger.LogAttrs(ctx, slog.LevelDebug, "restarting", slog.String("worker", worker.name), slog.Int("thread", handler.thread.threadIndex), slog.Int("exit_status", exitStatus)) return @@ -147,12 +150,12 @@ func tearDownWorkerScript(handler *workerThread, exitStatus int) { logger.LogAttrs(ctx, slog.LevelError, "worker script has not reached frankenphp_handle_request()", slog.String("worker", worker.name), slog.Int("thread", handler.thread.threadIndex)) // panic after exponential backoff if the worker has never reached frankenphp_handle_request - if handler.backoff.recordFailure() { + if handler.backoff.RecordFailure() { if !watcherIsEnabled && !handler.state.is(stateReady) { - logger.LogAttrs(ctx, slog.LevelError, "too many consecutive worker failures", slog.String("worker", worker.name), slog.Int("thread", handler.thread.threadIndex), slog.Int("failures", handler.backoff.failureCount)) + logger.LogAttrs(ctx, slog.LevelError, "too many consecutive worker failures", slog.String("worker", worker.name), slog.Int("thread", handler.thread.threadIndex), slog.Int("failures", handler.backoff.FailureCount())) panic("too many consecutive worker failures") } - logger.LogAttrs(ctx, slog.LevelWarn, "many consecutive worker failures", slog.String("worker", worker.name), slog.Int("thread", handler.thread.threadIndex), slog.Int("failures", handler.backoff.failureCount)) + logger.LogAttrs(ctx, slog.LevelWarn, "many consecutive worker failures", slog.String("worker", worker.name), slog.Int("thread", handler.thread.threadIndex), slog.Int("failures", handler.backoff.FailureCount())) } } From 5b7fbab3b195153586eb2ed92b40b4a772b05f2f Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Sat, 1 Nov 2025 21:34:46 +0100 Subject: [PATCH 02/31] Formatting. --- frankenphp.c | 5 +++-- internal/backoff/backoff.go | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/frankenphp.c b/frankenphp.c index 99eca7703f..a2b1d36a69 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -608,8 +608,9 @@ static char *frankenphp_read_cookies(void) { } /* all variables with well defined keys can safely be registered like this */ -static inline void frankenphp_register_trusted_var(zend_string *z_key, char *value, - size_t val_len, HashTable *ht) { +static inline void frankenphp_register_trusted_var(zend_string *z_key, + char *value, size_t val_len, + HashTable *ht) { if (value == NULL) { zval empty; ZVAL_EMPTY_STRING(&empty); diff --git a/internal/backoff/backoff.go b/internal/backoff/backoff.go index 9139623879..555eea99c2 100644 --- a/internal/backoff/backoff.go +++ b/internal/backoff/backoff.go @@ -55,4 +55,4 @@ func (e *ExponentialBackoff) FailureCount() int { defer e.mu.RUnlock() return e.failureCount -} \ No newline at end of file +} From 0cebb7479da06e0e1d2605cfb1e92fa0fa96909e Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Sat, 1 Nov 2025 22:05:21 +0100 Subject: [PATCH 03/31] Moves state to own module. --- debugstate.go | 14 ++- state.go => internal/state/state.go | 118 +++++++++--------- state_test.go => internal/state/state_test.go | 18 +-- phpmainthread.go | 25 ++-- phpmainthread_test.go | 7 +- phpthread.go | 34 ++--- scaling.go | 17 +-- scaling_test.go | 9 +- threadinactive.go | 22 ++-- threadregular.go | 22 ++-- threadtasks_test.go | 16 +-- threadworker.go | 35 +++--- worker.go | 9 +- 13 files changed, 185 insertions(+), 161 deletions(-) rename state.go => internal/state/state.go (54%) rename state_test.go => internal/state/state_test.go (74%) diff --git a/debugstate.go b/debugstate.go index a7941ac79c..d50f747040 100644 --- a/debugstate.go +++ b/debugstate.go @@ -1,5 +1,9 @@ package frankenphp +import ( + state "github.com/dunglas/frankenphp/internal/state" +) + // EXPERIMENTAL: ThreadDebugState prints the state of a single PHP thread - debugging purposes only type ThreadDebugState struct { Index int @@ -23,7 +27,7 @@ func DebugState() FrankenPHPDebugState { ReservedThreadCount: 0, } for _, thread := range phpThreads { - if thread.state.is(stateReserved) { + if thread.state.Is(state.StateReserved) { fullState.ReservedThreadCount++ continue } @@ -38,9 +42,9 @@ func threadDebugState(thread *phpThread) ThreadDebugState { return ThreadDebugState{ Index: thread.threadIndex, Name: thread.name(), - State: thread.state.name(), - IsWaiting: thread.state.isInWaitingState(), - IsBusy: !thread.state.isInWaitingState(), - WaitingSinceMilliseconds: thread.state.waitTime(), + State: thread.state.Name(), + IsWaiting: thread.state.IsInWaitingState(), + IsBusy: !thread.state.IsInWaitingState(), + WaitingSinceMilliseconds: thread.state.WaitTime(), } } diff --git a/state.go b/internal/state/state.go similarity index 54% rename from state.go rename to internal/state/state.go index 71970c8388..d7d811e4f5 100644 --- a/state.go +++ b/internal/state/state.go @@ -6,46 +6,46 @@ import ( "time" ) -type stateID uint8 +type StateID uint8 const ( - // livecycle states of a thread - stateReserved stateID = iota - stateBooting - stateBootRequested - stateShuttingDown - stateDone - - // these states are 'stable' and safe to transition from at any time - stateInactive - stateReady - - // states necessary for restarting workers - stateRestarting - stateYielding - - // states necessary for transitioning between different handlers - stateTransitionRequested - stateTransitionInProgress - stateTransitionComplete + // livecycle States of a thread + StateReserved StateID = iota + StateBooting + StateBootRequested + StateShuttingDown + StateDone + + // these States are 'stable' and safe to transition from at any time + StateInactive + StateReady + + // States necessary for restarting workers + StateRestarting + StateYielding + + // States necessary for transitioning between different handlers + StateTransitionRequested + StateTransitionInProgress + StateTransitionComplete ) -var stateNames = map[stateID]string{ - stateReserved: "reserved", - stateBooting: "booting", - stateInactive: "inactive", - stateReady: "ready", - stateShuttingDown: "shutting down", - stateDone: "done", - stateRestarting: "restarting", - stateYielding: "yielding", - stateTransitionRequested: "transition requested", - stateTransitionInProgress: "transition in progress", - stateTransitionComplete: "transition complete", -} - -type threadState struct { - currentState stateID +var stateNames = map[StateID]string{ + StateReserved: "reserved", + StateBooting: "booting", + StateInactive: "inactive", + StateReady: "ready", + StateShuttingDown: "shutting down", + StateDone: "done", + StateRestarting: "restarting", + StateYielding: "yielding", + StateTransitionRequested: "transition requested", + StateTransitionInProgress: "transition in progress", + StateTransitionComplete: "transition complete", +} + +type ThreadState struct { + currentState StateID mu sync.RWMutex subscribers []stateSubscriber // how long threads have been waiting in stable states @@ -54,19 +54,19 @@ type threadState struct { } type stateSubscriber struct { - states []stateID + states []StateID ch chan struct{} } -func newThreadState() *threadState { - return &threadState{ - currentState: stateReserved, +func NewThreadState() *ThreadState { + return &ThreadState{ + currentState: StateReserved, subscribers: []stateSubscriber{}, mu: sync.RWMutex{}, } } -func (ts *threadState) is(state stateID) bool { +func (ts *ThreadState) Is(state StateID) bool { ts.mu.RLock() ok := ts.currentState == state ts.mu.RUnlock() @@ -74,7 +74,7 @@ func (ts *threadState) is(state stateID) bool { return ok } -func (ts *threadState) compareAndSwap(compareTo stateID, swapTo stateID) bool { +func (ts *ThreadState) CompareAndSwap(compareTo StateID, swapTo StateID) bool { ts.mu.Lock() ok := ts.currentState == compareTo if ok { @@ -86,11 +86,11 @@ func (ts *threadState) compareAndSwap(compareTo stateID, swapTo stateID) bool { return ok } -func (ts *threadState) name() string { - return stateNames[ts.get()] +func (ts *ThreadState) Name() string { + return stateNames[ts.Get()] } -func (ts *threadState) get() stateID { +func (ts *ThreadState) Get() StateID { ts.mu.RLock() id := ts.currentState ts.mu.RUnlock() @@ -98,14 +98,14 @@ func (ts *threadState) get() stateID { return id } -func (ts *threadState) set(nextState stateID) { +func (ts *ThreadState) Set(nextState StateID) { ts.mu.Lock() ts.currentState = nextState ts.notifySubscribers(nextState) ts.mu.Unlock() } -func (ts *threadState) notifySubscribers(nextState stateID) { +func (ts *ThreadState) notifySubscribers(nextState StateID) { if len(ts.subscribers) == 0 { return } @@ -122,7 +122,7 @@ func (ts *threadState) notifySubscribers(nextState stateID) { } // block until the thread reaches a certain state -func (ts *threadState) waitFor(states ...stateID) { +func (ts *ThreadState) WaitFor(states ...StateID) { ts.mu.Lock() if slices.Contains(states, ts.currentState) { ts.mu.Unlock() @@ -138,15 +138,15 @@ func (ts *threadState) waitFor(states ...stateID) { } // safely request a state change from a different goroutine -func (ts *threadState) requestSafeStateChange(nextState stateID) bool { +func (ts *ThreadState) RequestSafeStateChange(nextState StateID) bool { ts.mu.Lock() switch ts.currentState { // disallow state changes if shutting down or done - case stateShuttingDown, stateDone, stateReserved: + case StateShuttingDown, StateDone, StateReserved: ts.mu.Unlock() return false // ready and inactive are safe states to transition from - case stateReady, stateInactive: + case StateReady, StateInactive: ts.currentState = nextState ts.notifySubscribers(nextState) ts.mu.Unlock() @@ -155,12 +155,12 @@ func (ts *threadState) requestSafeStateChange(nextState stateID) bool { ts.mu.Unlock() // wait for the state to change to a safe state - ts.waitFor(stateReady, stateInactive, stateShuttingDown) - return ts.requestSafeStateChange(nextState) + ts.WaitFor(StateReady, StateInactive, StateShuttingDown) + return ts.RequestSafeStateChange(nextState) } // markAsWaiting hints that the thread reached a stable state and is waiting for requests or shutdown -func (ts *threadState) markAsWaiting(isWaiting bool) { +func (ts *ThreadState) MarkAsWaiting(isWaiting bool) { ts.mu.Lock() if isWaiting { ts.isWaiting = true @@ -172,7 +172,7 @@ func (ts *threadState) markAsWaiting(isWaiting bool) { } // isWaitingState returns true if a thread is waiting for a request or shutdown -func (ts *threadState) isInWaitingState() bool { +func (ts *ThreadState) IsInWaitingState() bool { ts.mu.RLock() isWaiting := ts.isWaiting ts.mu.RUnlock() @@ -180,7 +180,7 @@ func (ts *threadState) isInWaitingState() bool { } // waitTime returns the time since the thread is waiting in a stable state in ms -func (ts *threadState) waitTime() int64 { +func (ts *ThreadState) WaitTime() int64 { ts.mu.RLock() waitTime := int64(0) if ts.isWaiting { @@ -189,3 +189,9 @@ func (ts *threadState) waitTime() int64 { ts.mu.RUnlock() return waitTime } + +func (ts *ThreadState) SetWaitTime(t time.Time) { + ts.mu.Lock() + ts.waitingSince = t + ts.mu.Unlock() +} diff --git a/state_test.go b/internal/state/state_test.go similarity index 74% rename from state_test.go rename to internal/state/state_test.go index 7055d35f65..9a5e844153 100644 --- a/state_test.go +++ b/internal/state/state_test.go @@ -11,13 +11,13 @@ func Test2GoroutinesYieldToEachOtherViaStates(t *testing.T) { threadState := &threadState{currentState: stateBooting} go func() { - threadState.waitFor(stateInactive) + threadState.WaitFor(stateInactive) assert.True(t, threadState.is(stateInactive)) - threadState.set(stateReady) + threadstate.Set(stateReady) }() - threadState.set(stateInactive) - threadState.waitFor(stateReady) + threadstate.Set(stateInactive) + threadState.WaitFor(stateReady) assert.True(t, threadState.is(stateReady)) } @@ -25,16 +25,16 @@ func TestStateShouldHaveCorrectAmountOfSubscribers(t *testing.T) { threadState := &threadState{currentState: stateBooting} // 3 subscribers waiting for different states - go threadState.waitFor(stateInactive) - go threadState.waitFor(stateInactive, stateShuttingDown) - go threadState.waitFor(stateShuttingDown) + go threadState.WaitFor(stateInactive) + go threadState.WaitFor(stateInactive, StateShuttingDown) + go threadState.WaitFor(StateShuttingDown) assertNumberOfSubscribers(t, threadState, 3) - threadState.set(stateInactive) + threadstate.Set(stateInactive) assertNumberOfSubscribers(t, threadState, 1) - assert.True(t, threadState.compareAndSwap(stateInactive, stateShuttingDown)) + assert.True(t, threadstate.CompareAndSwap(stateInactive, stateShuttingDown)) assertNumberOfSubscribers(t, threadState, 0) } diff --git a/phpmainthread.go b/phpmainthread.go index 3154bb77ba..b8081afe42 100644 --- a/phpmainthread.go +++ b/phpmainthread.go @@ -15,12 +15,13 @@ import ( "github.com/dunglas/frankenphp/internal/memory" "github.com/dunglas/frankenphp/internal/phpheaders" + state "github.com/dunglas/frankenphp/internal/state" ) // represents the main PHP thread // the thread needs to keep running as long as all other threads are running type phpMainThread struct { - state *threadState + state *state.ThreadState done chan struct{} numThreads int maxThreads int @@ -40,7 +41,7 @@ var ( // and reserves a fixed number of possible PHP threads func initPHPThreads(numThreads int, numMaxThreads int, phpIni map[string]string) (*phpMainThread, error) { mainThread = &phpMainThread{ - state: newThreadState(), + state: state.NewThreadState(), done: make(chan struct{}), numThreads: numThreads, maxThreads: numMaxThreads, @@ -84,11 +85,11 @@ func initPHPThreads(numThreads int, numMaxThreads int, phpIni map[string]string) func drainPHPThreads() { doneWG := sync.WaitGroup{} doneWG.Add(len(phpThreads)) - mainThread.state.set(stateShuttingDown) + mainThread.state.Set(state.StateShuttingDown) close(mainThread.done) for _, thread := range phpThreads { // shut down all reserved threads - if thread.state.compareAndSwap(stateReserved, stateDone) { + if thread.state.CompareAndSwap(state.StateReserved, state.StateDone) { doneWG.Done() continue } @@ -100,8 +101,8 @@ func drainPHPThreads() { } doneWG.Wait() - mainThread.state.set(stateDone) - mainThread.state.waitFor(stateReserved) + mainThread.state.Set(state.StateDone) + mainThread.state.WaitFor(state.StateReserved) phpThreads = nil } @@ -110,7 +111,7 @@ func (mainThread *phpMainThread) start() error { return ErrMainThreadCreation } - mainThread.state.waitFor(stateReady) + mainThread.state.WaitFor(state.StateReady) // cache common request headers as zend_strings (HTTP_ACCEPT, HTTP_USER_AGENT, etc.) mainThread.commonHeaders = make(map[string]*C.zend_string, len(phpheaders.CommonRequestHeaders)) @@ -129,13 +130,13 @@ func (mainThread *phpMainThread) start() error { func getInactivePHPThread() *phpThread { for _, thread := range phpThreads { - if thread.state.is(stateInactive) { + if thread.state.Is(state.StateInactive) { return thread } } for _, thread := range phpThreads { - if thread.state.compareAndSwap(stateReserved, stateBootRequested) { + if thread.state.CompareAndSwap(state.StateReserved, state.StateBootRequested) { thread.boot() return thread } @@ -151,8 +152,8 @@ func go_frankenphp_main_thread_is_ready() { mainThread.maxThreads = mainThread.numThreads } - mainThread.state.set(stateReady) - mainThread.state.waitFor(stateDone) + mainThread.state.Set(state.StateReady) + mainThread.state.WaitFor(state.StateDone) } // max_threads = auto @@ -176,7 +177,7 @@ func (mainThread *phpMainThread) setAutomaticMaxThreads() { //export go_frankenphp_shutdown_main_thread func go_frankenphp_shutdown_main_thread() { - mainThread.state.set(stateReserved) + mainThread.state.Set(state.StateReserved) } //export go_get_custom_php_ini diff --git a/phpmainthread_test.go b/phpmainthread_test.go index a49484b61f..75385e9f23 100644 --- a/phpmainthread_test.go +++ b/phpmainthread_test.go @@ -13,6 +13,7 @@ import ( "time" "github.com/dunglas/frankenphp/internal/phpheaders" + state "github.com/dunglas/frankenphp/internal/state" "github.com/stretchr/testify/assert" ) @@ -25,7 +26,7 @@ func TestStartAndStopTheMainThreadWithOneInactiveThread(t *testing.T) { assert.Len(t, phpThreads, 1) assert.Equal(t, 0, phpThreads[0].threadIndex) - assert.True(t, phpThreads[0].state.is(stateInactive)) + assert.True(t, phpThreads[0].state.Is(state.StateInactive)) drainPHPThreads() assert.Nil(t, phpThreads) @@ -159,7 +160,7 @@ func TestFinishBootingAWorkerScript(t *testing.T) { // boot the worker worker := getDummyWorker("transition-worker-1.php") convertToWorkerThread(phpThreads[0], worker) - phpThreads[0].state.waitFor(stateReady) + phpThreads[0].state.WaitFor(state.StateReady) assert.NotNil(t, phpThreads[0].handler.(*workerThread).dummyContext) assert.Nil(t, phpThreads[0].handler.(*workerThread).workerContext) @@ -225,7 +226,7 @@ func allPossibleTransitions(worker1Path string, worker2Path string) []func(*phpT convertToRegularThread, func(thread *phpThread) { thread.shutdown() }, func(thread *phpThread) { - if thread.state.is(stateReserved) { + if thread.state.Is(state.StateReserved) { thread.boot() } }, diff --git a/phpthread.go b/phpthread.go index 2691aa54bf..592e4f5756 100644 --- a/phpthread.go +++ b/phpthread.go @@ -9,6 +9,8 @@ import ( "runtime" "sync" "unsafe" + + state "github.com/dunglas/frankenphp/internal/state" ) // representation of the actual underlying PHP thread @@ -20,7 +22,7 @@ type phpThread struct { drainChan chan struct{} handlerMu sync.Mutex handler threadHandler - state *threadState + state *state.ThreadState sandboxedEnv map[string]*C.zend_string } @@ -36,16 +38,16 @@ func newPHPThread(threadIndex int) *phpThread { return &phpThread{ threadIndex: threadIndex, requestChan: make(chan *frankenPHPContext), - state: newThreadState(), + state: state.NewThreadState(), } } // boot starts the underlying PHP thread func (thread *phpThread) boot() { // thread must be in reserved state to boot - if !thread.state.compareAndSwap(stateReserved, stateBooting) && !thread.state.compareAndSwap(stateBootRequested, stateBooting) { - logger.Error("thread is not in reserved state: " + thread.state.name()) - panic("thread is not in reserved state: " + thread.state.name()) + if !thread.state.CompareAndSwap(state.StateReserved, state.StateBooting) && !thread.state.CompareAndSwap(state.StateBootRequested, state.StateBooting) { + logger.Error("thread is not in reserved state: " + thread.state.Name()) + panic("thread is not in reserved state: " + thread.state.Name()) } // boot threads as inactive @@ -60,22 +62,22 @@ func (thread *phpThread) boot() { panic("unable to create thread") } - thread.state.waitFor(stateInactive) + thread.state.WaitFor(state.StateInactive) } // shutdown the underlying PHP thread func (thread *phpThread) shutdown() { - if !thread.state.requestSafeStateChange(stateShuttingDown) { + if !thread.state.RequestSafeStateChange(state.StateShuttingDown) { // already shutting down or done return } close(thread.drainChan) - thread.state.waitFor(stateDone) + thread.state.WaitFor(state.StateDone) thread.drainChan = make(chan struct{}) // threads go back to the reserved state from which they can be booted again - if mainThread.state.is(stateReady) { - thread.state.set(stateReserved) + if mainThread.state.Is(state.StateReady) { + thread.state.Set(state.StateReserved) } } @@ -84,22 +86,22 @@ func (thread *phpThread) shutdown() { func (thread *phpThread) setHandler(handler threadHandler) { thread.handlerMu.Lock() defer thread.handlerMu.Unlock() - if !thread.state.requestSafeStateChange(stateTransitionRequested) { + if !thread.state.RequestSafeStateChange(state.StateTransitionRequested) { // no state change allowed == shutdown or done return } close(thread.drainChan) - thread.state.waitFor(stateTransitionInProgress) + thread.state.WaitFor(state.StateTransitionInProgress) thread.handler = handler thread.drainChan = make(chan struct{}) - thread.state.set(stateTransitionComplete) + thread.state.Set(state.StateTransitionComplete) } // transition to a new handler safely // is triggered by setHandler and executed on the PHP thread func (thread *phpThread) transitionToNewHandler() string { - thread.state.set(stateTransitionInProgress) - thread.state.waitFor(stateTransitionComplete) + thread.state.Set(state.StateTransitionInProgress) + thread.state.WaitFor(state.StateTransitionComplete) // execute beforeScriptExecution of the new handler return thread.handler.beforeScriptExecution() } @@ -166,5 +168,5 @@ func go_frankenphp_after_script_execution(threadIndex C.uintptr_t, exitStatus C. func go_frankenphp_on_thread_shutdown(threadIndex C.uintptr_t) { thread := phpThreads[threadIndex] thread.Unpin() - thread.state.set(stateDone) + thread.state.Set(state.StateDone) } diff --git a/scaling.go b/scaling.go index 57e6c598b9..6c70ccc41d 100644 --- a/scaling.go +++ b/scaling.go @@ -11,6 +11,7 @@ import ( "time" "github.com/dunglas/frankenphp/internal/cpu" + state "github.com/dunglas/frankenphp/internal/state" ) const ( @@ -64,7 +65,7 @@ func addRegularThread() (*phpThread, error) { return nil, ErrMaxThreadsReached } convertToRegularThread(thread) - thread.state.waitFor(stateReady, stateShuttingDown, stateReserved) + thread.state.WaitFor(state.StateReady, state.StateShuttingDown, state.StateReserved) return thread, nil } @@ -74,7 +75,7 @@ func addWorkerThread(worker *worker) (*phpThread, error) { return nil, ErrMaxThreadsReached } convertToWorkerThread(thread, worker) - thread.state.waitFor(stateReady, stateShuttingDown, stateReserved) + thread.state.WaitFor(state.StateReady, state.StateShuttingDown, state.StateReserved) return thread, nil } @@ -83,7 +84,7 @@ func scaleWorkerThread(worker *worker) { scalingMu.Lock() defer scalingMu.Unlock() - if !mainThread.state.is(stateReady) { + if !mainThread.state.Is(state.StateReady) { return } @@ -108,7 +109,7 @@ func scaleRegularThread() { scalingMu.Lock() defer scalingMu.Unlock() - if !mainThread.state.is(stateReady) { + if !mainThread.state.Is(state.StateReady) { return } @@ -189,18 +190,18 @@ func deactivateThreads() { thread := autoScaledThreads[i] // the thread might have been stopped otherwise, remove it - if thread.state.is(stateReserved) { + if thread.state.Is(state.StateReserved) { autoScaledThreads = append(autoScaledThreads[:i], autoScaledThreads[i+1:]...) continue } - waitTime := thread.state.waitTime() + waitTime := thread.state.WaitTime() if stoppedThreadCount > maxTerminationCount || waitTime == 0 { continue } // convert threads to inactive if they have been idle for too long - if thread.state.is(stateReady) && waitTime > maxThreadIdleTime.Milliseconds() { + if thread.state.Is(state.StateReady) && waitTime > maxThreadIdleTime.Milliseconds() { convertToInactiveThread(thread) stoppedThreadCount++ autoScaledThreads = append(autoScaledThreads[:i], autoScaledThreads[i+1:]...) @@ -212,7 +213,7 @@ func deactivateThreads() { // TODO: Completely stopping threads is more memory efficient // Some PECL extensions like #1296 will prevent threads from fully stopping (they leak memory) // Reactivate this if there is a better solution or workaround - // if thread.state.is(stateInactive) && waitTime > maxThreadIdleTime.Milliseconds() { + // if thread.state.Is(state.StateInactive) && waitTime > maxThreadIdleTime.Milliseconds() { // logger.LogAttrs(nil, slog.LevelDebug, "auto-stopping thread", slog.Int("thread", thread.threadIndex)) // thread.shutdown() // stoppedThreadCount++ diff --git a/scaling_test.go b/scaling_test.go index 89e04b51ee..f64c0f60c8 100644 --- a/scaling_test.go +++ b/scaling_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + state "github.com/dunglas/frankenphp/internal/state" "github.com/stretchr/testify/assert" ) @@ -20,7 +21,7 @@ func TestScaleARegularThreadUpAndDown(t *testing.T) { // scale up scaleRegularThread() - assert.Equal(t, stateReady, autoScaledThread.state.get()) + assert.Equal(t, state.StateReady, autoScaledThread.state.Get()) assert.IsType(t, ®ularThread{}, autoScaledThread.handler) // on down-scale, the thread will be marked as inactive @@ -49,7 +50,7 @@ func TestScaleAWorkerThreadUpAndDown(t *testing.T) { // scale up scaleWorkerThread(getWorkerByPath(workerPath)) - assert.Equal(t, stateReady, autoScaledThread.state.get()) + assert.Equal(t, state.StateReady, autoScaledThread.state.Get()) // on down-scale, the thread will be marked as inactive setLongWaitTime(autoScaledThread) @@ -60,7 +61,5 @@ func TestScaleAWorkerThreadUpAndDown(t *testing.T) { } func setLongWaitTime(thread *phpThread) { - thread.state.mu.Lock() - thread.state.waitingSince = time.Now().Add(-time.Hour) - thread.state.mu.Unlock() + thread.state.SetWaitTime(time.Now().Add(-time.Hour)) } diff --git a/threadinactive.go b/threadinactive.go index 912d339fee..42ac57a248 100644 --- a/threadinactive.go +++ b/threadinactive.go @@ -1,5 +1,9 @@ package frankenphp +import ( + state "github.com/dunglas/frankenphp/internal/state" +) + // representation of a thread with no work assigned to it // implements the threadHandler interface // each inactive thread weighs around ~350KB @@ -15,22 +19,22 @@ func convertToInactiveThread(thread *phpThread) { func (handler *inactiveThread) beforeScriptExecution() string { thread := handler.thread - switch thread.state.get() { - case stateTransitionRequested: + switch thread.state.Get() { + case state.StateTransitionRequested: return thread.transitionToNewHandler() - case stateBooting, stateTransitionComplete: - thread.state.set(stateInactive) + case state.StateBooting, state.StateTransitionComplete: + thread.state.Set(state.StateInactive) // wait for external signal to start or shut down - thread.state.markAsWaiting(true) - thread.state.waitFor(stateTransitionRequested, stateShuttingDown) - thread.state.markAsWaiting(false) + thread.state.MarkAsWaiting(true) + thread.state.WaitFor(state.StateTransitionRequested, state.StateShuttingDown) + thread.state.MarkAsWaiting(false) return handler.beforeScriptExecution() - case stateShuttingDown: + case state.StateShuttingDown: // signal to stop return "" } - panic("unexpected state: " + thread.state.name()) + panic("unexpected state: " + thread.state.Name()) } func (handler *inactiveThread) afterScriptExecution(int) { diff --git a/threadregular.go b/threadregular.go index a8ca03543c..bbcba0e0a1 100644 --- a/threadregular.go +++ b/threadregular.go @@ -2,13 +2,15 @@ package frankenphp import ( "sync" + + state "github.com/dunglas/frankenphp/internal/state" ) // representation of a non-worker PHP thread // executes PHP scripts in a web context // implements the threadHandler interface type regularThread struct { - state *threadState + state *state.ThreadState thread *phpThread requestContext *frankenPHPContext } @@ -29,22 +31,22 @@ func convertToRegularThread(thread *phpThread) { // beforeScriptExecution returns the name of the script or an empty string on shutdown func (handler *regularThread) beforeScriptExecution() string { - switch handler.state.get() { - case stateTransitionRequested: + switch handler.state.Get() { + case state.StateTransitionRequested: detachRegularThread(handler.thread) return handler.thread.transitionToNewHandler() - case stateTransitionComplete: + case state.StateTransitionComplete: handler.thread.updateContext(false) - handler.state.set(stateReady) + handler.state.Set(state.StateReady) return handler.waitForRequest() - case stateReady: + case state.StateReady: return handler.waitForRequest() - case stateShuttingDown: + case state.StateShuttingDown: detachRegularThread(handler.thread) // signal to stop return "" } - panic("unexpected state: " + handler.state.name()) + panic("unexpected state: " + handler.state.Name()) } func (handler *regularThread) afterScriptExecution(int) { @@ -63,7 +65,7 @@ func (handler *regularThread) waitForRequest() string { // clear any previously sandboxed env clearSandboxedEnv(handler.thread) - handler.state.markAsWaiting(true) + handler.state.MarkAsWaiting(true) var fc *frankenPHPContext select { @@ -74,7 +76,7 @@ func (handler *regularThread) waitForRequest() string { } handler.requestContext = fc - handler.state.markAsWaiting(false) + handler.state.MarkAsWaiting(false) // set the scriptFilename that should be executed return fc.scriptFilename diff --git a/threadtasks_test.go b/threadtasks_test.go index d81c555350..e774a41b12 100644 --- a/threadtasks_test.go +++ b/threadtasks_test.go @@ -2,6 +2,8 @@ package frankenphp import ( "sync" + + state "github.com/dunglas/frankenphp/internal/state" ) // representation of a thread that handles tasks directly assigned by go @@ -41,23 +43,23 @@ func convertToTaskThread(thread *phpThread) *taskThread { func (handler *taskThread) beforeScriptExecution() string { thread := handler.thread - switch thread.state.get() { - case stateTransitionRequested: + switch thread.state.Get() { + case state.StateTransitionRequested: return thread.transitionToNewHandler() - case stateBooting, stateTransitionComplete: - thread.state.set(stateReady) + case state.StateBooting, state.StateTransitionComplete: + thread.state.Set(state.StateReady) handler.waitForTasks() return handler.beforeScriptExecution() - case stateReady: + case state.StateReady: handler.waitForTasks() return handler.beforeScriptExecution() - case stateShuttingDown: + case state.StateShuttingDown: // signal to stop return "" } - panic("unexpected state: " + thread.state.name()) + panic("unexpected state: " + thread.state.Name()) } func (handler *taskThread) afterScriptExecution(int) { diff --git a/threadworker.go b/threadworker.go index 6d3ca6d2ce..fc70d7f452 100644 --- a/threadworker.go +++ b/threadworker.go @@ -11,13 +11,14 @@ import ( "unsafe" "github.com/dunglas/frankenphp/internal/backoff" + state "github.com/dunglas/frankenphp/internal/state" ) // representation of a thread assigned to a worker script // executes the PHP worker script in a loop // implements the threadHandler interface type workerThread struct { - state *threadState + state *state.ThreadState thread *phpThread worker *worker dummyContext *frankenPHPContext @@ -42,28 +43,28 @@ func convertToWorkerThread(thread *phpThread, worker *worker) { // beforeScriptExecution returns the name of the script or an empty string on shutdown func (handler *workerThread) beforeScriptExecution() string { - switch handler.state.get() { - case stateTransitionRequested: + switch handler.state.Get() { + case state.StateTransitionRequested: if handler.worker.onThreadShutdown != nil { handler.worker.onThreadShutdown(handler.thread.threadIndex) } handler.worker.detachThread(handler.thread) return handler.thread.transitionToNewHandler() - case stateRestarting: + case state.StateRestarting: if handler.worker.onThreadShutdown != nil { handler.worker.onThreadShutdown(handler.thread.threadIndex) } - handler.state.set(stateYielding) - handler.state.waitFor(stateReady, stateShuttingDown) + handler.state.Set(state.StateYielding) + handler.state.WaitFor(state.StateReady, state.StateShuttingDown) return handler.beforeScriptExecution() - case stateReady, stateTransitionComplete: + case state.StateReady, state.StateTransitionComplete: handler.thread.updateContext(true) if handler.worker.onThreadReady != nil { handler.worker.onThreadReady(handler.thread.threadIndex) } setupWorkerScript(handler, handler.worker) return handler.worker.fileName - case stateShuttingDown: + case state.StateShuttingDown: if handler.worker.onThreadShutdown != nil { handler.worker.onThreadShutdown(handler.thread.threadIndex) } @@ -71,7 +72,7 @@ func (handler *workerThread) beforeScriptExecution() string { // signal to stop return "" } - panic("unexpected state: " + handler.state.name()) + panic("unexpected state: " + handler.state.Name()) } func (handler *workerThread) afterScriptExecution(exitStatus int) { @@ -94,7 +95,7 @@ func setupWorkerScript(handler *workerThread, worker *worker) { handler.backoff.Wait() metrics.StartWorker(worker.name) - if handler.state.is(stateReady) { + if handler.state.Is(state.StateReady) { metrics.ReadyWorker(handler.worker.name) } @@ -151,7 +152,7 @@ func tearDownWorkerScript(handler *workerThread, exitStatus int) { // panic after exponential backoff if the worker has never reached frankenphp_handle_request if handler.backoff.RecordFailure() { - if !watcherIsEnabled && !handler.state.is(stateReady) { + if !watcherIsEnabled && !handler.state.Is(state.StateReady) { logger.LogAttrs(ctx, slog.LevelError, "too many consecutive worker failures", slog.String("worker", worker.name), slog.Int("thread", handler.thread.threadIndex), slog.Int("failures", handler.backoff.FailureCount())) panic("too many consecutive worker failures") } @@ -176,14 +177,14 @@ func (handler *workerThread) waitForWorkerRequest() (bool, any) { } // worker threads are 'ready' after they first reach frankenphp_handle_request() - // 'stateTransitionComplete' is only true on the first boot of the worker script, + // 'state.StateTransitionComplete' is only true on the first boot of the worker script, // while 'isBootingScript' is true on every boot of the worker script - if handler.state.is(stateTransitionComplete) { + if handler.state.Is(state.StateTransitionComplete) { metrics.ReadyWorker(handler.worker.name) - handler.state.set(stateReady) + handler.state.Set(state.StateReady) } - handler.state.markAsWaiting(true) + handler.state.MarkAsWaiting(true) var fc *frankenPHPContext select { @@ -192,7 +193,7 @@ func (handler *workerThread) waitForWorkerRequest() (bool, any) { // flush the opcache when restarting due to watcher or admin api // note: this is done right before frankenphp_handle_request() returns 'false' - if handler.state.is(stateRestarting) { + if handler.state.Is(state.StateRestarting) { C.frankenphp_reset_opcache() } @@ -202,7 +203,7 @@ func (handler *workerThread) waitForWorkerRequest() (bool, any) { } handler.workerContext = fc - handler.state.markAsWaiting(false) + handler.state.MarkAsWaiting(false) if fc.request == nil { logger.LogAttrs(ctx, slog.LevelDebug, "request handling started", slog.String("worker", handler.worker.name), slog.Int("thread", handler.thread.threadIndex)) diff --git a/worker.go b/worker.go index ee468bd6cd..f44a154e46 100644 --- a/worker.go +++ b/worker.go @@ -9,6 +9,7 @@ import ( "time" "github.com/dunglas/frankenphp/internal/fastabs" + state "github.com/dunglas/frankenphp/internal/state" "github.com/dunglas/frankenphp/internal/watcher" ) @@ -52,7 +53,7 @@ func initWorkers(opt []workerOpt) error { thread := getInactivePHPThread() convertToWorkerThread(thread, w) go func() { - thread.state.waitFor(stateReady) + thread.state.WaitFor(state.StateReady) workersReady.Done() }() } @@ -146,7 +147,7 @@ func drainWorkerThreads() []*phpThread { worker.threadMutex.RLock() ready.Add(len(worker.threads)) for _, thread := range worker.threads { - if !thread.state.requestSafeStateChange(stateRestarting) { + if !thread.state.RequestSafeStateChange(state.StateRestarting) { ready.Done() // no state change allowed == thread is shutting down // we'll proceed to restart all other threads anyways @@ -155,7 +156,7 @@ func drainWorkerThreads() []*phpThread { close(thread.drainChan) drainedThreads = append(drainedThreads, thread) go func(thread *phpThread) { - thread.state.waitFor(stateYielding) + thread.state.WaitFor(state.StateYielding) ready.Done() }(thread) } @@ -182,7 +183,7 @@ func RestartWorkers() { for _, thread := range threadsToRestart { thread.drainChan = make(chan struct{}) - thread.state.set(stateReady) + thread.state.Set(state.StateReady) } } From 6dc34328ba8686116383a89af481f2d35ec27d20 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Sat, 1 Nov 2025 22:13:02 +0100 Subject: [PATCH 04/31] Refactoring. --- debugstate.go | 4 +-- internal/state/state.go | 56 ++++++++++++++++++------------------ internal/state/state_test.go | 31 ++++++++++---------- phpmainthread.go | 22 +++++++------- phpmainthread_test.go | 8 +++--- phpthread.go | 26 ++++++++--------- scaling.go | 16 +++++------ scaling_test.go | 6 ++-- threadinactive.go | 12 ++++---- threadregular.go | 12 ++++---- threadtasks_test.go | 12 ++++---- threadworker.go | 26 ++++++++--------- worker.go | 10 +++---- 13 files changed, 121 insertions(+), 120 deletions(-) diff --git a/debugstate.go b/debugstate.go index d50f747040..c18813ec1b 100644 --- a/debugstate.go +++ b/debugstate.go @@ -1,7 +1,7 @@ package frankenphp import ( - state "github.com/dunglas/frankenphp/internal/state" + "github.com/dunglas/frankenphp/internal/state" ) // EXPERIMENTAL: ThreadDebugState prints the state of a single PHP thread - debugging purposes only @@ -27,7 +27,7 @@ func DebugState() FrankenPHPDebugState { ReservedThreadCount: 0, } for _, thread := range phpThreads { - if thread.state.Is(state.StateReserved) { + if thread.state.Is(state.Reserved) { fullState.ReservedThreadCount++ continue } diff --git a/internal/state/state.go b/internal/state/state.go index d7d811e4f5..33b365a672 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -1,4 +1,4 @@ -package frankenphp +package state import ( "slices" @@ -10,38 +10,38 @@ type StateID uint8 const ( // livecycle States of a thread - StateReserved StateID = iota - StateBooting - StateBootRequested - StateShuttingDown - StateDone + Reserved StateID = iota + Booting + BootRequested + ShuttingDown + Done // these States are 'stable' and safe to transition from at any time - StateInactive - StateReady + Inactive + Ready // States necessary for restarting workers - StateRestarting - StateYielding + Restarting + Yielding // States necessary for transitioning between different handlers - StateTransitionRequested - StateTransitionInProgress - StateTransitionComplete + TransitionRequested + TransitionInProgress + TransitionComplete ) var stateNames = map[StateID]string{ - StateReserved: "reserved", - StateBooting: "booting", - StateInactive: "inactive", - StateReady: "ready", - StateShuttingDown: "shutting down", - StateDone: "done", - StateRestarting: "restarting", - StateYielding: "yielding", - StateTransitionRequested: "transition requested", - StateTransitionInProgress: "transition in progress", - StateTransitionComplete: "transition complete", + Reserved: "reserved", + Booting: "booting", + Inactive: "inactive", + Ready: "ready", + ShuttingDown: "shutting down", + Done: "done", + Restarting: "restarting", + Yielding: "yielding", + TransitionRequested: "transition requested", + TransitionInProgress: "transition in progress", + TransitionComplete: "transition complete", } type ThreadState struct { @@ -60,7 +60,7 @@ type stateSubscriber struct { func NewThreadState() *ThreadState { return &ThreadState{ - currentState: StateReserved, + currentState: Reserved, subscribers: []stateSubscriber{}, mu: sync.RWMutex{}, } @@ -142,11 +142,11 @@ func (ts *ThreadState) RequestSafeStateChange(nextState StateID) bool { ts.mu.Lock() switch ts.currentState { // disallow state changes if shutting down or done - case StateShuttingDown, StateDone, StateReserved: + case ShuttingDown, Done, Reserved: ts.mu.Unlock() return false // ready and inactive are safe states to transition from - case StateReady, StateInactive: + case Ready, Inactive: ts.currentState = nextState ts.notifySubscribers(nextState) ts.mu.Unlock() @@ -155,7 +155,7 @@ func (ts *ThreadState) RequestSafeStateChange(nextState StateID) bool { ts.mu.Unlock() // wait for the state to change to a safe state - ts.WaitFor(StateReady, StateInactive, StateShuttingDown) + ts.WaitFor(Ready, Inactive, ShuttingDown) return ts.RequestSafeStateChange(nextState) } diff --git a/internal/state/state_test.go b/internal/state/state_test.go index 9a5e844153..3da5266038 100644 --- a/internal/state/state_test.go +++ b/internal/state/state_test.go @@ -1,4 +1,4 @@ -package frankenphp +package state import ( "testing" @@ -8,37 +8,38 @@ import ( ) func Test2GoroutinesYieldToEachOtherViaStates(t *testing.T) { - threadState := &threadState{currentState: stateBooting} + threadState := &ThreadState{currentState: Booting} go func() { - threadState.WaitFor(stateInactive) - assert.True(t, threadState.is(stateInactive)) - threadstate.Set(stateReady) + threadState.WaitFor(Inactive) + assert.True(t, threadState.Is(Inactive)) + threadState.Set(Ready) }() - threadstate.Set(stateInactive) - threadState.WaitFor(stateReady) - assert.True(t, threadState.is(stateReady)) + threadState.Set(Inactive) + threadState.WaitFor(Ready) + assert.True(t, threadState.Is(Ready)) } func TestStateShouldHaveCorrectAmountOfSubscribers(t *testing.T) { - threadState := &threadState{currentState: stateBooting} + threadState := &ThreadState{currentState: Booting} // 3 subscribers waiting for different states - go threadState.WaitFor(stateInactive) - go threadState.WaitFor(stateInactive, StateShuttingDown) - go threadState.WaitFor(StateShuttingDown) + go threadState.WaitFor(Inactive) + go threadState.WaitFor(Inactive, ShuttingDown) + go threadState.WaitFor(ShuttingDown) assertNumberOfSubscribers(t, threadState, 3) - threadstate.Set(stateInactive) + threadState.Set(Inactive) assertNumberOfSubscribers(t, threadState, 1) - assert.True(t, threadstate.CompareAndSwap(stateInactive, stateShuttingDown)) + assert.True(t, threadState.CompareAndSwap(Inactive, ShuttingDown)) assertNumberOfSubscribers(t, threadState, 0) } -func assertNumberOfSubscribers(t *testing.T, threadState *threadState, expected int) { +func assertNumberOfSubscribers(t *testing.T, threadState *ThreadState, expected int) { + t.Helper() for range 10_000 { // wait for 1 second max time.Sleep(100 * time.Microsecond) threadState.mu.RLock() diff --git a/phpmainthread.go b/phpmainthread.go index b8081afe42..a2299a7092 100644 --- a/phpmainthread.go +++ b/phpmainthread.go @@ -15,7 +15,7 @@ import ( "github.com/dunglas/frankenphp/internal/memory" "github.com/dunglas/frankenphp/internal/phpheaders" - state "github.com/dunglas/frankenphp/internal/state" + "github.com/dunglas/frankenphp/internal/state" ) // represents the main PHP thread @@ -85,11 +85,11 @@ func initPHPThreads(numThreads int, numMaxThreads int, phpIni map[string]string) func drainPHPThreads() { doneWG := sync.WaitGroup{} doneWG.Add(len(phpThreads)) - mainThread.state.Set(state.StateShuttingDown) + mainThread.state.Set(state.ShuttingDown) close(mainThread.done) for _, thread := range phpThreads { // shut down all reserved threads - if thread.state.CompareAndSwap(state.StateReserved, state.StateDone) { + if thread.state.CompareAndSwap(state.Reserved, state.Done) { doneWG.Done() continue } @@ -101,8 +101,8 @@ func drainPHPThreads() { } doneWG.Wait() - mainThread.state.Set(state.StateDone) - mainThread.state.WaitFor(state.StateReserved) + mainThread.state.Set(state.Done) + mainThread.state.WaitFor(state.Reserved) phpThreads = nil } @@ -111,7 +111,7 @@ func (mainThread *phpMainThread) start() error { return ErrMainThreadCreation } - mainThread.state.WaitFor(state.StateReady) + mainThread.state.WaitFor(state.Ready) // cache common request headers as zend_strings (HTTP_ACCEPT, HTTP_USER_AGENT, etc.) mainThread.commonHeaders = make(map[string]*C.zend_string, len(phpheaders.CommonRequestHeaders)) @@ -130,13 +130,13 @@ func (mainThread *phpMainThread) start() error { func getInactivePHPThread() *phpThread { for _, thread := range phpThreads { - if thread.state.Is(state.StateInactive) { + if thread.state.Is(state.Inactive) { return thread } } for _, thread := range phpThreads { - if thread.state.CompareAndSwap(state.StateReserved, state.StateBootRequested) { + if thread.state.CompareAndSwap(state.Reserved, state.BootRequested) { thread.boot() return thread } @@ -152,8 +152,8 @@ func go_frankenphp_main_thread_is_ready() { mainThread.maxThreads = mainThread.numThreads } - mainThread.state.Set(state.StateReady) - mainThread.state.WaitFor(state.StateDone) + mainThread.state.Set(state.Ready) + mainThread.state.WaitFor(state.Done) } // max_threads = auto @@ -177,7 +177,7 @@ func (mainThread *phpMainThread) setAutomaticMaxThreads() { //export go_frankenphp_shutdown_main_thread func go_frankenphp_shutdown_main_thread() { - mainThread.state.Set(state.StateReserved) + mainThread.state.Set(state.Reserved) } //export go_get_custom_php_ini diff --git a/phpmainthread_test.go b/phpmainthread_test.go index 75385e9f23..3cb7de132e 100644 --- a/phpmainthread_test.go +++ b/phpmainthread_test.go @@ -13,7 +13,7 @@ import ( "time" "github.com/dunglas/frankenphp/internal/phpheaders" - state "github.com/dunglas/frankenphp/internal/state" + "github.com/dunglas/frankenphp/internal/state" "github.com/stretchr/testify/assert" ) @@ -26,7 +26,7 @@ func TestStartAndStopTheMainThreadWithOneInactiveThread(t *testing.T) { assert.Len(t, phpThreads, 1) assert.Equal(t, 0, phpThreads[0].threadIndex) - assert.True(t, phpThreads[0].state.Is(state.StateInactive)) + assert.True(t, phpThreads[0].state.Is(state.Inactive)) drainPHPThreads() assert.Nil(t, phpThreads) @@ -160,7 +160,7 @@ func TestFinishBootingAWorkerScript(t *testing.T) { // boot the worker worker := getDummyWorker("transition-worker-1.php") convertToWorkerThread(phpThreads[0], worker) - phpThreads[0].state.WaitFor(state.StateReady) + phpThreads[0].state.WaitFor(state.Ready) assert.NotNil(t, phpThreads[0].handler.(*workerThread).dummyContext) assert.Nil(t, phpThreads[0].handler.(*workerThread).workerContext) @@ -226,7 +226,7 @@ func allPossibleTransitions(worker1Path string, worker2Path string) []func(*phpT convertToRegularThread, func(thread *phpThread) { thread.shutdown() }, func(thread *phpThread) { - if thread.state.Is(state.StateReserved) { + if thread.state.Is(state.Reserved) { thread.boot() } }, diff --git a/phpthread.go b/phpthread.go index 592e4f5756..7af9e13acc 100644 --- a/phpthread.go +++ b/phpthread.go @@ -10,7 +10,7 @@ import ( "sync" "unsafe" - state "github.com/dunglas/frankenphp/internal/state" + "github.com/dunglas/frankenphp/internal/state" ) // representation of the actual underlying PHP thread @@ -45,7 +45,7 @@ func newPHPThread(threadIndex int) *phpThread { // boot starts the underlying PHP thread func (thread *phpThread) boot() { // thread must be in reserved state to boot - if !thread.state.CompareAndSwap(state.StateReserved, state.StateBooting) && !thread.state.CompareAndSwap(state.StateBootRequested, state.StateBooting) { + if !thread.state.CompareAndSwap(state.Reserved, state.Booting) && !thread.state.CompareAndSwap(state.BootRequested, state.Booting) { logger.Error("thread is not in reserved state: " + thread.state.Name()) panic("thread is not in reserved state: " + thread.state.Name()) } @@ -62,22 +62,22 @@ func (thread *phpThread) boot() { panic("unable to create thread") } - thread.state.WaitFor(state.StateInactive) + thread.state.WaitFor(state.Inactive) } // shutdown the underlying PHP thread func (thread *phpThread) shutdown() { - if !thread.state.RequestSafeStateChange(state.StateShuttingDown) { + if !thread.state.RequestSafeStateChange(state.ShuttingDown) { // already shutting down or done return } close(thread.drainChan) - thread.state.WaitFor(state.StateDone) + thread.state.WaitFor(state.Done) thread.drainChan = make(chan struct{}) // threads go back to the reserved state from which they can be booted again - if mainThread.state.Is(state.StateReady) { - thread.state.Set(state.StateReserved) + if mainThread.state.Is(state.Ready) { + thread.state.Set(state.Reserved) } } @@ -86,22 +86,22 @@ func (thread *phpThread) shutdown() { func (thread *phpThread) setHandler(handler threadHandler) { thread.handlerMu.Lock() defer thread.handlerMu.Unlock() - if !thread.state.RequestSafeStateChange(state.StateTransitionRequested) { + if !thread.state.RequestSafeStateChange(state.TransitionRequested) { // no state change allowed == shutdown or done return } close(thread.drainChan) - thread.state.WaitFor(state.StateTransitionInProgress) + thread.state.WaitFor(state.TransitionInProgress) thread.handler = handler thread.drainChan = make(chan struct{}) - thread.state.Set(state.StateTransitionComplete) + thread.state.Set(state.TransitionComplete) } // transition to a new handler safely // is triggered by setHandler and executed on the PHP thread func (thread *phpThread) transitionToNewHandler() string { - thread.state.Set(state.StateTransitionInProgress) - thread.state.WaitFor(state.StateTransitionComplete) + thread.state.Set(state.TransitionInProgress) + thread.state.WaitFor(state.TransitionComplete) // execute beforeScriptExecution of the new handler return thread.handler.beforeScriptExecution() } @@ -168,5 +168,5 @@ func go_frankenphp_after_script_execution(threadIndex C.uintptr_t, exitStatus C. func go_frankenphp_on_thread_shutdown(threadIndex C.uintptr_t) { thread := phpThreads[threadIndex] thread.Unpin() - thread.state.Set(state.StateDone) + thread.state.Set(state.Done) } diff --git a/scaling.go b/scaling.go index 6c70ccc41d..a2c253063a 100644 --- a/scaling.go +++ b/scaling.go @@ -11,7 +11,7 @@ import ( "time" "github.com/dunglas/frankenphp/internal/cpu" - state "github.com/dunglas/frankenphp/internal/state" + "github.com/dunglas/frankenphp/internal/state" ) const ( @@ -65,7 +65,7 @@ func addRegularThread() (*phpThread, error) { return nil, ErrMaxThreadsReached } convertToRegularThread(thread) - thread.state.WaitFor(state.StateReady, state.StateShuttingDown, state.StateReserved) + thread.state.WaitFor(state.Ready, state.ShuttingDown, state.Reserved) return thread, nil } @@ -75,7 +75,7 @@ func addWorkerThread(worker *worker) (*phpThread, error) { return nil, ErrMaxThreadsReached } convertToWorkerThread(thread, worker) - thread.state.WaitFor(state.StateReady, state.StateShuttingDown, state.StateReserved) + thread.state.WaitFor(state.Ready, state.ShuttingDown, state.Reserved) return thread, nil } @@ -84,7 +84,7 @@ func scaleWorkerThread(worker *worker) { scalingMu.Lock() defer scalingMu.Unlock() - if !mainThread.state.Is(state.StateReady) { + if !mainThread.state.Is(state.Ready) { return } @@ -109,7 +109,7 @@ func scaleRegularThread() { scalingMu.Lock() defer scalingMu.Unlock() - if !mainThread.state.Is(state.StateReady) { + if !mainThread.state.Is(state.Ready) { return } @@ -190,7 +190,7 @@ func deactivateThreads() { thread := autoScaledThreads[i] // the thread might have been stopped otherwise, remove it - if thread.state.Is(state.StateReserved) { + if thread.state.Is(state.Reserved) { autoScaledThreads = append(autoScaledThreads[:i], autoScaledThreads[i+1:]...) continue } @@ -201,7 +201,7 @@ func deactivateThreads() { } // convert threads to inactive if they have been idle for too long - if thread.state.Is(state.StateReady) && waitTime > maxThreadIdleTime.Milliseconds() { + if thread.state.Is(state.Ready) && waitTime > maxThreadIdleTime.Milliseconds() { convertToInactiveThread(thread) stoppedThreadCount++ autoScaledThreads = append(autoScaledThreads[:i], autoScaledThreads[i+1:]...) @@ -213,7 +213,7 @@ func deactivateThreads() { // TODO: Completely stopping threads is more memory efficient // Some PECL extensions like #1296 will prevent threads from fully stopping (they leak memory) // Reactivate this if there is a better solution or workaround - // if thread.state.Is(state.StateInactive) && waitTime > maxThreadIdleTime.Milliseconds() { + // if thread.state.Is(state.Inactive) && waitTime > maxThreadIdleTime.Milliseconds() { // logger.LogAttrs(nil, slog.LevelDebug, "auto-stopping thread", slog.Int("thread", thread.threadIndex)) // thread.shutdown() // stoppedThreadCount++ diff --git a/scaling_test.go b/scaling_test.go index f64c0f60c8..da7a4d2992 100644 --- a/scaling_test.go +++ b/scaling_test.go @@ -6,7 +6,7 @@ import ( "testing" "time" - state "github.com/dunglas/frankenphp/internal/state" + "github.com/dunglas/frankenphp/internal/state" "github.com/stretchr/testify/assert" ) @@ -21,7 +21,7 @@ func TestScaleARegularThreadUpAndDown(t *testing.T) { // scale up scaleRegularThread() - assert.Equal(t, state.StateReady, autoScaledThread.state.Get()) + assert.Equal(t, state.Ready, autoScaledThread.state.Get()) assert.IsType(t, ®ularThread{}, autoScaledThread.handler) // on down-scale, the thread will be marked as inactive @@ -50,7 +50,7 @@ func TestScaleAWorkerThreadUpAndDown(t *testing.T) { // scale up scaleWorkerThread(getWorkerByPath(workerPath)) - assert.Equal(t, state.StateReady, autoScaledThread.state.Get()) + assert.Equal(t, state.Ready, autoScaledThread.state.Get()) // on down-scale, the thread will be marked as inactive setLongWaitTime(autoScaledThread) diff --git a/threadinactive.go b/threadinactive.go index 42ac57a248..f825c7a185 100644 --- a/threadinactive.go +++ b/threadinactive.go @@ -1,7 +1,7 @@ package frankenphp import ( - state "github.com/dunglas/frankenphp/internal/state" + "github.com/dunglas/frankenphp/internal/state" ) // representation of a thread with no work assigned to it @@ -20,17 +20,17 @@ func (handler *inactiveThread) beforeScriptExecution() string { thread := handler.thread switch thread.state.Get() { - case state.StateTransitionRequested: + case state.TransitionRequested: return thread.transitionToNewHandler() - case state.StateBooting, state.StateTransitionComplete: - thread.state.Set(state.StateInactive) + case state.Booting, state.TransitionComplete: + thread.state.Set(state.Inactive) // wait for external signal to start or shut down thread.state.MarkAsWaiting(true) - thread.state.WaitFor(state.StateTransitionRequested, state.StateShuttingDown) + thread.state.WaitFor(state.TransitionRequested, state.ShuttingDown) thread.state.MarkAsWaiting(false) return handler.beforeScriptExecution() - case state.StateShuttingDown: + case state.ShuttingDown: // signal to stop return "" } diff --git a/threadregular.go b/threadregular.go index bbcba0e0a1..82e5ca3600 100644 --- a/threadregular.go +++ b/threadregular.go @@ -3,7 +3,7 @@ package frankenphp import ( "sync" - state "github.com/dunglas/frankenphp/internal/state" + "github.com/dunglas/frankenphp/internal/state" ) // representation of a non-worker PHP thread @@ -32,16 +32,16 @@ func convertToRegularThread(thread *phpThread) { // beforeScriptExecution returns the name of the script or an empty string on shutdown func (handler *regularThread) beforeScriptExecution() string { switch handler.state.Get() { - case state.StateTransitionRequested: + case state.TransitionRequested: detachRegularThread(handler.thread) return handler.thread.transitionToNewHandler() - case state.StateTransitionComplete: + case state.TransitionComplete: handler.thread.updateContext(false) - handler.state.Set(state.StateReady) + handler.state.Set(state.Ready) return handler.waitForRequest() - case state.StateReady: + case state.Ready: return handler.waitForRequest() - case state.StateShuttingDown: + case state.ShuttingDown: detachRegularThread(handler.thread) // signal to stop return "" diff --git a/threadtasks_test.go b/threadtasks_test.go index e774a41b12..65e97bb6b4 100644 --- a/threadtasks_test.go +++ b/threadtasks_test.go @@ -3,7 +3,7 @@ package frankenphp import ( "sync" - state "github.com/dunglas/frankenphp/internal/state" + "github.com/dunglas/frankenphp/internal/state" ) // representation of a thread that handles tasks directly assigned by go @@ -44,18 +44,18 @@ func (handler *taskThread) beforeScriptExecution() string { thread := handler.thread switch thread.state.Get() { - case state.StateTransitionRequested: + case state.TransitionRequested: return thread.transitionToNewHandler() - case state.StateBooting, state.StateTransitionComplete: - thread.state.Set(state.StateReady) + case state.Booting, state.TransitionComplete: + thread.state.Set(state.Ready) handler.waitForTasks() return handler.beforeScriptExecution() - case state.StateReady: + case state.Ready: handler.waitForTasks() return handler.beforeScriptExecution() - case state.StateShuttingDown: + case state.ShuttingDown: // signal to stop return "" } diff --git a/threadworker.go b/threadworker.go index fc70d7f452..c50e917ddd 100644 --- a/threadworker.go +++ b/threadworker.go @@ -11,7 +11,7 @@ import ( "unsafe" "github.com/dunglas/frankenphp/internal/backoff" - state "github.com/dunglas/frankenphp/internal/state" + "github.com/dunglas/frankenphp/internal/state" ) // representation of a thread assigned to a worker script @@ -44,27 +44,27 @@ func convertToWorkerThread(thread *phpThread, worker *worker) { // beforeScriptExecution returns the name of the script or an empty string on shutdown func (handler *workerThread) beforeScriptExecution() string { switch handler.state.Get() { - case state.StateTransitionRequested: + case state.TransitionRequested: if handler.worker.onThreadShutdown != nil { handler.worker.onThreadShutdown(handler.thread.threadIndex) } handler.worker.detachThread(handler.thread) return handler.thread.transitionToNewHandler() - case state.StateRestarting: + case state.Restarting: if handler.worker.onThreadShutdown != nil { handler.worker.onThreadShutdown(handler.thread.threadIndex) } - handler.state.Set(state.StateYielding) - handler.state.WaitFor(state.StateReady, state.StateShuttingDown) + handler.state.Set(state.Yielding) + handler.state.WaitFor(state.Ready, state.ShuttingDown) return handler.beforeScriptExecution() - case state.StateReady, state.StateTransitionComplete: + case state.Ready, state.TransitionComplete: handler.thread.updateContext(true) if handler.worker.onThreadReady != nil { handler.worker.onThreadReady(handler.thread.threadIndex) } setupWorkerScript(handler, handler.worker) return handler.worker.fileName - case state.StateShuttingDown: + case state.ShuttingDown: if handler.worker.onThreadShutdown != nil { handler.worker.onThreadShutdown(handler.thread.threadIndex) } @@ -95,7 +95,7 @@ func setupWorkerScript(handler *workerThread, worker *worker) { handler.backoff.Wait() metrics.StartWorker(worker.name) - if handler.state.Is(state.StateReady) { + if handler.state.Is(state.Ready) { metrics.ReadyWorker(handler.worker.name) } @@ -152,7 +152,7 @@ func tearDownWorkerScript(handler *workerThread, exitStatus int) { // panic after exponential backoff if the worker has never reached frankenphp_handle_request if handler.backoff.RecordFailure() { - if !watcherIsEnabled && !handler.state.Is(state.StateReady) { + if !watcherIsEnabled && !handler.state.Is(state.Ready) { logger.LogAttrs(ctx, slog.LevelError, "too many consecutive worker failures", slog.String("worker", worker.name), slog.Int("thread", handler.thread.threadIndex), slog.Int("failures", handler.backoff.FailureCount())) panic("too many consecutive worker failures") } @@ -177,11 +177,11 @@ func (handler *workerThread) waitForWorkerRequest() (bool, any) { } // worker threads are 'ready' after they first reach frankenphp_handle_request() - // 'state.StateTransitionComplete' is only true on the first boot of the worker script, + // 'state.TransitionComplete' is only true on the first boot of the worker script, // while 'isBootingScript' is true on every boot of the worker script - if handler.state.Is(state.StateTransitionComplete) { + if handler.state.Is(state.TransitionComplete) { metrics.ReadyWorker(handler.worker.name) - handler.state.Set(state.StateReady) + handler.state.Set(state.Ready) } handler.state.MarkAsWaiting(true) @@ -193,7 +193,7 @@ func (handler *workerThread) waitForWorkerRequest() (bool, any) { // flush the opcache when restarting due to watcher or admin api // note: this is done right before frankenphp_handle_request() returns 'false' - if handler.state.Is(state.StateRestarting) { + if handler.state.Is(state.Restarting) { C.frankenphp_reset_opcache() } diff --git a/worker.go b/worker.go index f44a154e46..96ba45e338 100644 --- a/worker.go +++ b/worker.go @@ -9,7 +9,7 @@ import ( "time" "github.com/dunglas/frankenphp/internal/fastabs" - state "github.com/dunglas/frankenphp/internal/state" + "github.com/dunglas/frankenphp/internal/state" "github.com/dunglas/frankenphp/internal/watcher" ) @@ -53,7 +53,7 @@ func initWorkers(opt []workerOpt) error { thread := getInactivePHPThread() convertToWorkerThread(thread, w) go func() { - thread.state.WaitFor(state.StateReady) + thread.state.WaitFor(state.Ready) workersReady.Done() }() } @@ -147,7 +147,7 @@ func drainWorkerThreads() []*phpThread { worker.threadMutex.RLock() ready.Add(len(worker.threads)) for _, thread := range worker.threads { - if !thread.state.RequestSafeStateChange(state.StateRestarting) { + if !thread.state.RequestSafeStateChange(state.Restarting) { ready.Done() // no state change allowed == thread is shutting down // we'll proceed to restart all other threads anyways @@ -156,7 +156,7 @@ func drainWorkerThreads() []*phpThread { close(thread.drainChan) drainedThreads = append(drainedThreads, thread) go func(thread *phpThread) { - thread.state.WaitFor(state.StateYielding) + thread.state.WaitFor(state.Yielding) ready.Done() }(thread) } @@ -183,7 +183,7 @@ func RestartWorkers() { for _, thread := range threadsToRestart { thread.drainChan = make(chan struct{}) - thread.state.Set(state.StateReady) + thread.state.Set(state.Ready) } } From 7c8813ee6d39eeafbecaa07fae54fbb586faa61a Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Sat, 1 Nov 2025 22:37:35 +0100 Subject: [PATCH 05/31] Also moves php_headers test. --- internal/phpheaders/phpheaders_test.go | 22 ++++++++++++++++++++++ phpmainthread_test.go | 15 --------------- 2 files changed, 22 insertions(+), 15 deletions(-) create mode 100644 internal/phpheaders/phpheaders_test.go diff --git a/internal/phpheaders/phpheaders_test.go b/internal/phpheaders/phpheaders_test.go new file mode 100644 index 0000000000..a741ec38fc --- /dev/null +++ b/internal/phpheaders/phpheaders_test.go @@ -0,0 +1,22 @@ +package phpheaders + +import ( + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestAllCommonHeadersAreCorrect(t *testing.T) { + fakeRequest := httptest.NewRequest("GET", "http://localhost", nil) + + for header, phpHeader := range CommonRequestHeaders { + // verify that common and uncommon headers return the same result + expectedPHPHeader := GetUnCommonHeader(header) + assert.Equal(t, phpHeader+"\x00", expectedPHPHeader, "header is not well formed: "+phpHeader) + + // net/http will capitalize lowercase headers, verify that headers are capitalized + fakeRequest.Header.Add(header, "foo") + assert.Contains(t, fakeRequest.Header, header, "header is not correctly capitalized: "+header) + } +} diff --git a/phpmainthread_test.go b/phpmainthread_test.go index 3cb7de132e..81d94c2d35 100644 --- a/phpmainthread_test.go +++ b/phpmainthread_test.go @@ -12,7 +12,6 @@ import ( "testing" "time" - "github.com/dunglas/frankenphp/internal/phpheaders" "github.com/dunglas/frankenphp/internal/state" "github.com/stretchr/testify/assert" ) @@ -237,20 +236,6 @@ func allPossibleTransitions(worker1Path string, worker2Path string) []func(*phpT } } -func TestAllCommonHeadersAreCorrect(t *testing.T) { - fakeRequest := httptest.NewRequest("GET", "http://localhost", nil) - - for header, phpHeader := range phpheaders.CommonRequestHeaders { - // verify that common and uncommon headers return the same result - expectedPHPHeader := phpheaders.GetUnCommonHeader(header) - assert.Equal(t, phpHeader+"\x00", expectedPHPHeader, "header is not well formed: "+phpHeader) - - // net/http will capitalize lowercase headers, verify that headers are capitalized - fakeRequest.Header.Add(header, "foo") - assert.Contains(t, fakeRequest.Header, header, "header is not correctly capitalized: "+header) - } -} - func TestCorrectThreadCalculation(t *testing.T) { maxProcs := runtime.GOMAXPROCS(0) * 2 oneWorkerThread := []workerOpt{{num: 1}} From 1436802e7f49f04639e73a11dbca091c0dfb8c6d Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Thu, 6 Nov 2025 22:39:28 +0100 Subject: [PATCH 06/31] tests with -vet=off --- .github/workflows/docker.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/docker.yaml b/.github/workflows/docker.yaml index 97bed1630a..a755bb9e55 100644 --- a/.github/workflows/docker.yaml +++ b/.github/workflows/docker.yaml @@ -213,7 +213,7 @@ jobs: run: | docker run --platform="${PLATFORM}" --rm \ "$(jq -r ".\"builder-${VARIANT}\".\"containerimage.config.digest\"" <<< "${METADATA}")" \ - sh -c "./go.sh test ${RACE} -v $(./go.sh list ./... | grep -v github.com/dunglas/frankenphp/internal/testext | grep -v github.com/dunglas/frankenphp/internal/extgen | tr '\n' ' ') && cd caddy && ../go.sh test ${RACE} -v ./..." + sh -c "./go.sh test ${RACE} -v -vet=off $(./go.sh list ./... | grep -v github.com/dunglas/frankenphp/internal/testext | grep -v github.com/dunglas/frankenphp/internal/extgen | tr '\n' ' ') && cd caddy && ../go.sh test ${RACE} -v ./..." env: METADATA: ${{ steps.build.outputs.metadata }} PLATFORM: ${{ matrix.platform }} From 7c79d7a463946560dc3a96db69a9b25e808fccb6 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Thu, 6 Nov 2025 22:51:41 +0100 Subject: [PATCH 07/31] tests with ./go.sh vet before. --- .github/workflows/docker.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/docker.yaml b/.github/workflows/docker.yaml index a755bb9e55..b32e45f5b1 100644 --- a/.github/workflows/docker.yaml +++ b/.github/workflows/docker.yaml @@ -213,7 +213,7 @@ jobs: run: | docker run --platform="${PLATFORM}" --rm \ "$(jq -r ".\"builder-${VARIANT}\".\"containerimage.config.digest\"" <<< "${METADATA}")" \ - sh -c "./go.sh test ${RACE} -v -vet=off $(./go.sh list ./... | grep -v github.com/dunglas/frankenphp/internal/testext | grep -v github.com/dunglas/frankenphp/internal/extgen | tr '\n' ' ') && cd caddy && ../go.sh test ${RACE} -v ./..." + sh -c "./go.sh vet ./... && ./go.sh test ${RACE} -v $(./go.sh list ./... | grep -v github.com/dunglas/frankenphp/internal/testext | grep -v github.com/dunglas/frankenphp/internal/extgen | tr '\n' ' ') && cd caddy && ../go.sh test ${RACE} -v ./..." env: METADATA: ${{ steps.build.outputs.metadata }} PLATFORM: ${{ matrix.platform }} From f5bb4e02e3f854d7fd6ff130361ae5ed0271b161 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Sat, 8 Nov 2025 22:04:10 +0100 Subject: [PATCH 08/31] Moves env logic to the C side. --- env.go | 114 ++++++++--------------------------------------- frankenphp.c | 110 ++++++++++++++++++++++----------------------- frankenphp.h | 2 - phpmainthread.go | 12 +++-- phpthread.go | 13 +++--- threadregular.go | 3 -- threadworker.go | 1 - 7 files changed, 84 insertions(+), 171 deletions(-) diff --git a/env.go b/env.go index 9e6fbfdfe5..8d74aeac26 100644 --- a/env.go +++ b/env.go @@ -1,9 +1,7 @@ package frankenphp // #cgo nocallback frankenphp_init_persistent_string -// #cgo nocallback frankenphp_add_assoc_str_ex // #cgo noescape frankenphp_init_persistent_string -// #cgo noescape frankenphp_add_assoc_str_ex // #include "frankenphp.h" import "C" import ( @@ -12,106 +10,30 @@ import ( "unsafe" ) -func initializeEnv() map[string]*C.zend_string { - env := os.Environ() - envMap := make(map[string]*C.zend_string, len(env)) - - for _, envVar := range env { - key, val, _ := strings.Cut(envVar, "=") - envMap[key] = C.frankenphp_init_persistent_string(toUnsafeChar(val), C.size_t(len(val))) - } - - return envMap -} - -// get the main thread env or the thread specific env -func getSandboxedEnv(thread *phpThread) map[string]*C.zend_string { - if thread.sandboxedEnv != nil { - return thread.sandboxedEnv - } - - return mainThread.sandboxedEnv -} - -func clearSandboxedEnv(thread *phpThread) { - if thread.sandboxedEnv == nil { - return - } - - for key, val := range thread.sandboxedEnv { - valInMainThread, ok := mainThread.sandboxedEnv[key] - if !ok || val != valInMainThread { - C.free(unsafe.Pointer(val)) - } - } - - thread.sandboxedEnv = nil -} - -// if an env var already exists, it needs to be freed -func removeEnvFromThread(thread *phpThread, key string) { - valueInThread, existsInThread := thread.sandboxedEnv[key] - if !existsInThread { - return - } - - valueInMainThread, ok := mainThread.sandboxedEnv[key] - if !ok || valueInThread != valueInMainThread { - C.free(unsafe.Pointer(valueInThread)) - } - - delete(thread.sandboxedEnv, key) -} - -// copy the main thread env to the thread specific env -func cloneSandboxedEnv(thread *phpThread) { - if thread.sandboxedEnv != nil { - return - } - thread.sandboxedEnv = make(map[string]*C.zend_string, len(mainThread.sandboxedEnv)) - for key, value := range mainThread.sandboxedEnv { - thread.sandboxedEnv[key] = value - } -} - //export go_putenv -func go_putenv(threadIndex C.uintptr_t, str *C.char, length C.int) C.bool { - thread := phpThreads[threadIndex] - envString := C.GoStringN(str, length) - cloneSandboxedEnv(thread) +func go_putenv(name *C.char, nameLen C.int, val *C.char, valLen C.int) C.bool { + goName := C.GoStringN(name, nameLen) - // Check if '=' is present in the string - if key, val, found := strings.Cut(envString, "="); found { - removeEnvFromThread(thread, key) - thread.sandboxedEnv[key] = C.frankenphp_init_persistent_string(toUnsafeChar(val), C.size_t(len(val))) - return os.Setenv(key, val) == nil + if val == nil { + // Unset the environment variable + return C.bool(os.Unsetenv(goName) == nil) } - // No '=', unset the environment variable - removeEnvFromThread(thread, envString) - return os.Unsetenv(envString) == nil -} - -//export go_getfullenv -func go_getfullenv(threadIndex C.uintptr_t, trackVarsArray *C.zval) { - thread := phpThreads[threadIndex] - env := getSandboxedEnv(thread) - - for key, val := range env { - C.frankenphp_add_assoc_str_ex(trackVarsArray, toUnsafeChar(key), C.size_t(len(key)), val) - } + goVal := C.GoStringN(val, valLen) + return C.bool(os.Setenv(goName, goVal) == nil) } -//export go_getenv -func go_getenv(threadIndex C.uintptr_t, name *C.char) (C.bool, *C.zend_string) { - thread := phpThreads[threadIndex] +//export go_init_os_env +func go_init_os_env(trackVarsArray *C.HashTable) { + env := os.Environ() + for _, envVar := range env { + key, val, _ := strings.Cut(envVar, "=") + zkey := C.frankenphp_init_persistent_string(toUnsafeChar(key), C.size_t(len(key))) + zvalStr := C.frankenphp_init_persistent_string(toUnsafeChar(val), C.size_t(len(val))) - // Get the environment variable value - envValue, exists := getSandboxedEnv(thread)[C.GoString(name)] - if !exists { - // Environment variable does not exist - return false, nil // Return 0 to indicate failure + var zval C.zval + *(*uint32)(unsafe.Pointer(&zval.u1)) = C.IS_INTERNED_STRING_EX + *(**C.zend_string)(unsafe.Pointer(&zval.value)) = zvalStr + C.zend_hash_update(trackVarsArray, zkey, &zval) } - - return true, envValue // Return 1 to indicate success } diff --git a/frankenphp.c b/frankenphp.c index 3d124eced1..8aaa28bc85 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -71,9 +71,11 @@ frankenphp_config frankenphp_get_config() { } bool should_filter_var = 0; +HashTable *main_thread_env = NULL; + __thread uintptr_t thread_index; __thread bool is_worker_thread = false; -__thread zval *os_environment = NULL; +__thread HashTable *sandboxed_env = NULL; static void frankenphp_update_request_context() { /* the server context is stored on the go side, still SG(server_context) needs @@ -203,12 +205,7 @@ bool frankenphp_shutdown_dummy_request(void) { } PHPAPI void get_full_env(zval *track_vars_array) { - go_getfullenv(thread_index, track_vars_array); -} - -void frankenphp_add_assoc_str_ex(zval *track_vars_array, char *key, - size_t keylen, zend_string *val) { - add_assoc_str_ex(track_vars_array, key, keylen, val); + zend_hash_copy(Z_ARR_P(track_vars_array), main_thread_env, NULL); } /* Adapted from php_request_startup() */ @@ -305,39 +302,55 @@ PHP_FUNCTION(frankenphp_putenv) { RETURN_FALSE; } - if (go_putenv(thread_index, setting, (int)setting_len)) { - RETURN_TRUE; + if (sandboxed_env == NULL) { + sandboxed_env = zend_array_dup(main_thread_env); + } + + // cut the string at the first '=' + char *eq_pos = strchr(setting, '='); + bool put_env_success = true; + if (eq_pos != NULL) { + size_t name_len = eq_pos - setting; + size_t value_len = + (setting_len > name_len + 1) ? (setting_len - name_len - 1) : 0; + put_env_success = + go_putenv(setting, (int)name_len, eq_pos + 1, (int)value_len); + zval val = {0}; + ZVAL_STRINGL(&val, eq_pos + 1, value_len); + zend_hash_str_update(sandboxed_env, setting, name_len, &val); } else { - RETURN_FALSE; + // no '=' found, delete the variable + put_env_success = go_putenv(setting, (int)setting_len, NULL, 0); + zend_hash_str_del(sandboxed_env, setting, setting_len); } + + RETURN_BOOL(put_env_success); } /* }}} */ /* {{{ Call go's getenv to prevent race conditions */ PHP_FUNCTION(frankenphp_getenv) { - char *name = NULL; - size_t name_len = 0; + zend_string *name = NULL; bool local_only = 0; ZEND_PARSE_PARAMETERS_START(0, 2) Z_PARAM_OPTIONAL - Z_PARAM_STRING_OR_NULL(name, name_len) + Z_PARAM_STR_OR_NULL(name) Z_PARAM_BOOL(local_only) ZEND_PARSE_PARAMETERS_END(); - if (!name) { - array_init(return_value); - get_full_env(return_value); + HashTable *ht = sandboxed_env ? sandboxed_env : main_thread_env; + if (!name) { + RETURN_ARR(zend_array_dup(ht)); return; } - struct go_getenv_return result = go_getenv(thread_index, name); - - if (result.r0) { - // Return the single environment variable as a string - RETVAL_STR(result.r1); + zval *env_val = zend_hash_find(ht, name); + if (env_val && Z_TYPE_P(env_val) == IS_STRING) { + zend_string *str = Z_STR_P(env_val); + zend_string_addref(str); + RETVAL_STR(str); } else { - // Environment variable does not exist RETVAL_FALSE; } } /* }}} */ @@ -546,6 +559,10 @@ static zend_module_entry frankenphp_module = { STANDARD_MODULE_PROPERTIES}; static void frankenphp_request_shutdown() { + if (sandboxed_env != NULL) { + zend_hash_release(sandboxed_env); + sandboxed_env = NULL; + } frankenphp_free_request_context(); php_request_shutdown((void *)0); } @@ -774,29 +791,7 @@ static void frankenphp_register_variables(zval *track_vars_array) { /* In CGI mode, we consider the environment to be a part of the server * variables. */ - - /* in non-worker mode we import the os environment regularly */ - if (!is_worker_thread) { - get_full_env(track_vars_array); - // php_import_environment_variables(track_vars_array); - go_register_variables(thread_index, track_vars_array); - return; - } - - /* In worker mode we cache the os environment */ - if (os_environment == NULL) { - os_environment = malloc(sizeof(zval)); - if (os_environment == NULL) { - php_error(E_ERROR, "Failed to allocate memory for os_environment"); - - return; - } - array_init(os_environment); - get_full_env(os_environment); - // php_import_environment_variables(os_environment); - } - zend_hash_copy(Z_ARR_P(track_vars_array), Z_ARR_P(os_environment), - (copy_ctor_func_t)zval_add_ref); + zend_hash_copy(Z_ARR_P(track_vars_array), main_thread_env, NULL); go_register_variables(thread_index, track_vars_array); } @@ -806,10 +801,12 @@ static void frankenphp_log_message(const char *message, int syslog_type_int) { } static char *frankenphp_getenv(const char *name, size_t name_len) { - struct go_getenv_return result = go_getenv(thread_index, (char *)name); + HashTable *ht = sandboxed_env ? sandboxed_env : main_thread_env; - if (result.r0) { - return result.r1->val; + zval *env_val = zend_hash_str_find(ht, name, name_len); + if (env_val && Z_TYPE_P(env_val) == IS_STRING) { + zend_string *str = Z_STR_P(env_val); + return ZSTR_VAL(str); } return NULL; @@ -945,8 +942,18 @@ static void *php_main(void *arg) { cfg_get_string("filter.default", &default_filter); should_filter_var = default_filter != NULL; + if (main_thread_env == NULL) { + main_thread_env = malloc(sizeof(HashTable)); + zend_hash_init(main_thread_env, 8, NULL, NULL, 1); + go_init_os_env(main_thread_env); + } + go_frankenphp_main_thread_is_ready(); + // free env and entries + zend_hash_release(main_thread_env); + main_thread_env = NULL; + /* channel closed, shutdown gracefully */ frankenphp_sapi_module.shutdown(&frankenphp_sapi_module); @@ -1018,13 +1025,6 @@ int frankenphp_execute_script(char *file_name) { zend_catch { status = EG(exit_status); } zend_end_try(); - // free the cached os environment before shutting down the script - if (os_environment != NULL) { - zval_ptr_dtor(os_environment); - free(os_environment); - os_environment = NULL; - } - zend_destroy_file_handle(&file_handle); frankenphp_request_shutdown(); diff --git a/frankenphp.h b/frankenphp.h index c17df6061a..c574a9470c 100644 --- a/frankenphp.h +++ b/frankenphp.h @@ -65,8 +65,6 @@ void frankenphp_register_variable_safe(char *key, char *var, size_t val_len, zend_string *frankenphp_init_persistent_string(const char *string, size_t len); int frankenphp_reset_opcache(void); int frankenphp_get_current_memory_limit(); -void frankenphp_add_assoc_str_ex(zval *track_vars_array, char *key, - size_t keylen, zend_string *val); void frankenphp_register_single(zend_string *z_key, char *value, size_t val_len, zval *track_vars_array); diff --git a/phpmainthread.go b/phpmainthread.go index 3154bb77ba..12fff6d935 100644 --- a/phpmainthread.go +++ b/phpmainthread.go @@ -27,7 +27,6 @@ type phpMainThread struct { phpIni map[string]string commonHeaders map[string]*C.zend_string knownServerKeys map[string]*C.zend_string - sandboxedEnv map[string]*C.zend_string } var ( @@ -40,12 +39,11 @@ var ( // and reserves a fixed number of possible PHP threads func initPHPThreads(numThreads int, numMaxThreads int, phpIni map[string]string) (*phpMainThread, error) { mainThread = &phpMainThread{ - state: newThreadState(), - done: make(chan struct{}), - numThreads: numThreads, - maxThreads: numMaxThreads, - phpIni: phpIni, - sandboxedEnv: initializeEnv(), + state: newThreadState(), + done: make(chan struct{}), + numThreads: numThreads, + maxThreads: numMaxThreads, + phpIni: phpIni, } // initialize the first thread diff --git a/phpthread.go b/phpthread.go index a60aa8f0aa..271ab6cdc6 100644 --- a/phpthread.go +++ b/phpthread.go @@ -15,13 +15,12 @@ import ( // identified by the index in the phpThreads slice type phpThread struct { runtime.Pinner - threadIndex int - requestChan chan *frankenPHPContext - drainChan chan struct{} - handlerMu sync.Mutex - handler threadHandler - state *threadState - sandboxedEnv map[string]*C.zend_string + threadIndex int + requestChan chan *frankenPHPContext + drainChan chan struct{} + handlerMu sync.Mutex + handler threadHandler + state *threadState } // interface that defines how the callbacks from the C thread should be handled diff --git a/threadregular.go b/threadregular.go index 88cef7e79d..d41e8a46b4 100644 --- a/threadregular.go +++ b/threadregular.go @@ -59,9 +59,6 @@ func (handler *regularThread) name() string { } func (handler *regularThread) waitForRequest() string { - // clear any previously sandboxed env - clearSandboxedEnv(handler.thread) - handler.state.markAsWaiting(true) var fc *frankenPHPContext diff --git a/threadworker.go b/threadworker.go index 5a59f9278b..9fb407a5ec 100644 --- a/threadworker.go +++ b/threadworker.go @@ -108,7 +108,6 @@ func setupWorkerScript(handler *workerThread, worker *worker) { fc.worker = worker handler.dummyContext = fc handler.isBootingScript = true - clearSandboxedEnv(handler.thread) logger.LogAttrs(context.Background(), slog.LevelDebug, "starting", slog.String("worker", worker.name), slog.Int("thread", handler.thread.threadIndex)) } From 06a329b6795baaa025440e0c818b1ef74cb4f55b Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Sat, 8 Nov 2025 23:27:42 +0100 Subject: [PATCH 09/31] Cleanup. --- env.go | 26 +++++++++++++------------- frankenphp.c | 11 ++++++----- worker_test.go | 4 +--- 3 files changed, 20 insertions(+), 21 deletions(-) diff --git a/env.go b/env.go index 8d74aeac26..51758c8f36 100644 --- a/env.go +++ b/env.go @@ -10,19 +10,6 @@ import ( "unsafe" ) -//export go_putenv -func go_putenv(name *C.char, nameLen C.int, val *C.char, valLen C.int) C.bool { - goName := C.GoStringN(name, nameLen) - - if val == nil { - // Unset the environment variable - return C.bool(os.Unsetenv(goName) == nil) - } - - goVal := C.GoStringN(val, valLen) - return C.bool(os.Setenv(goName, goVal) == nil) -} - //export go_init_os_env func go_init_os_env(trackVarsArray *C.HashTable) { env := os.Environ() @@ -37,3 +24,16 @@ func go_init_os_env(trackVarsArray *C.HashTable) { C.zend_hash_update(trackVarsArray, zkey, &zval) } } + +//export go_putenv +func go_putenv(name *C.char, nameLen C.int, val *C.char, valLen C.int) C.bool { + goName := C.GoStringN(name, nameLen) + + if val == nil { + // Unset the environment variable + return C.bool(os.Unsetenv(goName) == nil) + } + + goVal := C.GoStringN(val, valLen) + return C.bool(os.Setenv(goName, goVal) == nil) +} diff --git a/frankenphp.c b/frankenphp.c index 8aaa28bc85..d5c7d66e59 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -327,7 +327,7 @@ PHP_FUNCTION(frankenphp_putenv) { RETURN_BOOL(put_env_success); } /* }}} */ -/* {{{ Call go's getenv to prevent race conditions */ +/* {{{ Get the env from the sandboxed environment */ PHP_FUNCTION(frankenphp_getenv) { zend_string *name = NULL; bool local_only = 0; @@ -942,6 +942,7 @@ static void *php_main(void *arg) { cfg_get_string("filter.default", &default_filter); should_filter_var = default_filter != NULL; + /* take a snapshot of the environment for sandboxing */ if (main_thread_env == NULL) { main_thread_env = malloc(sizeof(HashTable)); zend_hash_init(main_thread_env, 8, NULL, NULL, 1); @@ -950,10 +951,6 @@ static void *php_main(void *arg) { go_frankenphp_main_thread_is_ready(); - // free env and entries - zend_hash_release(main_thread_env); - main_thread_env = NULL; - /* channel closed, shutdown gracefully */ frankenphp_sapi_module.shutdown(&frankenphp_sapi_module); @@ -967,6 +964,10 @@ static void *php_main(void *arg) { frankenphp_sapi_module.ini_entries = NULL; } + /* free the main thread environment, necessary for tests */ + zend_hash_release(main_thread_env); + main_thread_env = NULL; + go_frankenphp_shutdown_main_thread(); return NULL; diff --git a/worker_test.go b/worker_test.go index c6b4245d47..fe17d2c422 100644 --- a/worker_test.go +++ b/worker_test.go @@ -13,8 +13,6 @@ import ( "strings" "testing" - "github.com/stretchr/testify/require" - "github.com/dunglas/frankenphp" "github.com/stretchr/testify/assert" "go.uber.org/zap/exp/zapslog" @@ -150,7 +148,7 @@ func ExampleServeHTTP_workers() { } func TestWorkerHasOSEnvironmentVariableInSERVER(t *testing.T) { - require.NoError(t, os.Setenv("CUSTOM_OS_ENV_VARIABLE", "custom_env_variable_value")) + assert.NoError(t, os.Setenv("CUSTOM_OS_ENV_VARIABLE", "custom_env_variable_value")) runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) { req := httptest.NewRequest("GET", "http://example.com/worker.php", nil) From 87123bd4095cadfb6c4fd40c9ff9e115298790ed Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Mon, 10 Nov 2025 19:26:45 +0100 Subject: [PATCH 10/31] import C test. --- .github/workflows/docker.yaml | 2 +- internal/backoff/backoff.go | 1 + internal/phpheaders/phpheaders.go | 1 + internal/state/state.go | 1 + 4 files changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/docker.yaml b/.github/workflows/docker.yaml index b32e45f5b1..97bed1630a 100644 --- a/.github/workflows/docker.yaml +++ b/.github/workflows/docker.yaml @@ -213,7 +213,7 @@ jobs: run: | docker run --platform="${PLATFORM}" --rm \ "$(jq -r ".\"builder-${VARIANT}\".\"containerimage.config.digest\"" <<< "${METADATA}")" \ - sh -c "./go.sh vet ./... && ./go.sh test ${RACE} -v $(./go.sh list ./... | grep -v github.com/dunglas/frankenphp/internal/testext | grep -v github.com/dunglas/frankenphp/internal/extgen | tr '\n' ' ') && cd caddy && ../go.sh test ${RACE} -v ./..." + sh -c "./go.sh test ${RACE} -v $(./go.sh list ./... | grep -v github.com/dunglas/frankenphp/internal/testext | grep -v github.com/dunglas/frankenphp/internal/extgen | tr '\n' ' ') && cd caddy && ../go.sh test ${RACE} -v ./..." env: METADATA: ${{ steps.build.outputs.metadata }} PLATFORM: ${{ matrix.platform }} diff --git a/internal/backoff/backoff.go b/internal/backoff/backoff.go index 555eea99c2..73182f4a37 100644 --- a/internal/backoff/backoff.go +++ b/internal/backoff/backoff.go @@ -1,5 +1,6 @@ package backoff +import "C" import ( "sync" "time" diff --git a/internal/phpheaders/phpheaders.go b/internal/phpheaders/phpheaders.go index 64bc3a0832..5d7709baad 100644 --- a/internal/phpheaders/phpheaders.go +++ b/internal/phpheaders/phpheaders.go @@ -1,5 +1,6 @@ package phpheaders +import "C" import ( "strings" diff --git a/internal/state/state.go b/internal/state/state.go index 33b365a672..5c6d5dadc8 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -1,5 +1,6 @@ package state +import "C" import ( "slices" "sync" From 4161623736b02759f5a1867bc465a400e284dced Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Tue, 11 Nov 2025 21:07:50 +0100 Subject: [PATCH 11/31] adds turns state into string --- internal/state/state.go | 60 ++++++++++++++++------------------------- 1 file changed, 23 insertions(+), 37 deletions(-) diff --git a/internal/state/state.go b/internal/state/state.go index 5c6d5dadc8..52c2d0b156 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -7,46 +7,32 @@ import ( "time" ) -type StateID uint8 +type State string const ( // livecycle States of a thread - Reserved StateID = iota - Booting - BootRequested - ShuttingDown - Done + Reserved State = "reserved" + Booting State = "booting" + BootRequested State = "boot requested" + ShuttingDown State = "shutting down" + Done State = "done" // these States are 'stable' and safe to transition from at any time - Inactive - Ready + Inactive State = "inactive" + Ready State = "ready" // States necessary for restarting workers - Restarting - Yielding + Restarting State = "restarting" + Yielding State = "yielding" // States necessary for transitioning between different handlers - TransitionRequested - TransitionInProgress - TransitionComplete + TransitionRequested State = "transition requested" + TransitionInProgress State = "transition in progress" + TransitionComplete State = "transition complete" ) -var stateNames = map[StateID]string{ - Reserved: "reserved", - Booting: "booting", - Inactive: "inactive", - Ready: "ready", - ShuttingDown: "shutting down", - Done: "done", - Restarting: "restarting", - Yielding: "yielding", - TransitionRequested: "transition requested", - TransitionInProgress: "transition in progress", - TransitionComplete: "transition complete", -} - type ThreadState struct { - currentState StateID + currentState State mu sync.RWMutex subscribers []stateSubscriber // how long threads have been waiting in stable states @@ -55,7 +41,7 @@ type ThreadState struct { } type stateSubscriber struct { - states []StateID + states []State ch chan struct{} } @@ -67,7 +53,7 @@ func NewThreadState() *ThreadState { } } -func (ts *ThreadState) Is(state StateID) bool { +func (ts *ThreadState) Is(state State) bool { ts.mu.RLock() ok := ts.currentState == state ts.mu.RUnlock() @@ -75,7 +61,7 @@ func (ts *ThreadState) Is(state StateID) bool { return ok } -func (ts *ThreadState) CompareAndSwap(compareTo StateID, swapTo StateID) bool { +func (ts *ThreadState) CompareAndSwap(compareTo State, swapTo State) bool { ts.mu.Lock() ok := ts.currentState == compareTo if ok { @@ -88,10 +74,10 @@ func (ts *ThreadState) CompareAndSwap(compareTo StateID, swapTo StateID) bool { } func (ts *ThreadState) Name() string { - return stateNames[ts.Get()] + return string(ts.Get()) } -func (ts *ThreadState) Get() StateID { +func (ts *ThreadState) Get() State { ts.mu.RLock() id := ts.currentState ts.mu.RUnlock() @@ -99,14 +85,14 @@ func (ts *ThreadState) Get() StateID { return id } -func (ts *ThreadState) Set(nextState StateID) { +func (ts *ThreadState) Set(nextState State) { ts.mu.Lock() ts.currentState = nextState ts.notifySubscribers(nextState) ts.mu.Unlock() } -func (ts *ThreadState) notifySubscribers(nextState StateID) { +func (ts *ThreadState) notifySubscribers(nextState State) { if len(ts.subscribers) == 0 { return } @@ -123,7 +109,7 @@ func (ts *ThreadState) notifySubscribers(nextState StateID) { } // block until the thread reaches a certain state -func (ts *ThreadState) WaitFor(states ...StateID) { +func (ts *ThreadState) WaitFor(states ...State) { ts.mu.Lock() if slices.Contains(states, ts.currentState) { ts.mu.Unlock() @@ -139,7 +125,7 @@ func (ts *ThreadState) WaitFor(states ...StateID) { } // safely request a state change from a different goroutine -func (ts *ThreadState) RequestSafeStateChange(nextState StateID) bool { +func (ts *ThreadState) RequestSafeStateChange(nextState State) bool { ts.mu.Lock() switch ts.currentState { // disallow state changes if shutting down or done From a36547bc2ffb620ec6cc43f4c6ff4a4e276ef858 Mon Sep 17 00:00:00 2001 From: Alexander Stecher <45872305+AlliBalliBaba@users.noreply.github.com> Date: Thu, 13 Nov 2025 23:38:54 +0100 Subject: [PATCH 12/31] suggestion: simplify exponential backoff (#1970) * removes backoff. * Adjusts comment. * Suggestions by @dunglas * Removes 'max_consecutive_failures' * Removes 'max_consecutive_failures' * Adjusts warning. * Disables the logger in tests. * Revert "Adjusts warning." This reverts commit e93a6a930129e938d076fc5176a283f6b4b45852. * Revert "Removes 'max_consecutive_failures'" This reverts commit ba28ea0e4ada8639095bac8273961edf4c3b4cb2. * Revert "Removes 'max_consecutive_failures'" This reverts commit 32e649caf7a0f0b987cac3ffd37f6855497355d7. * Only fails on max failures again. * Restores failure timings. --- frankenphp_test.go | 10 +++--- internal/backoff/backoff.go | 59 -------------------------------- internal/backoff/backoff_test.go | 41 ---------------------- phpmainthread_test.go | 13 ++++--- testdata/failing-worker.php | 17 ++------- threadworker.go | 41 +++++++++++++--------- worker.go | 25 ++++++++++---- 7 files changed, 57 insertions(+), 149 deletions(-) delete mode 100644 internal/backoff/backoff.go delete mode 100644 internal/backoff/backoff_test.go diff --git a/frankenphp_test.go b/frankenphp_test.go index 7b4b44dbce..f7e0a171cf 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -601,10 +601,12 @@ func testRequestHeaders(t *testing.T, opts *testOptions) { } func TestFailingWorker(t *testing.T) { - runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) { - body, _ := testGet("http://example.com/failing-worker.php", handler, t) - assert.Contains(t, body, "ok") - }, &testOptions{workerScript: "failing-worker.php"}) + err := frankenphp.Init( + frankenphp.WithLogger(slog.New(slog.NewTextHandler(io.Discard, nil))), + frankenphp.WithWorkers("failing worker", "testdata/failing-worker.php", 4, frankenphp.WithWorkerMaxFailures(1)), + frankenphp.WithNumThreads(5), + ) + assert.Error(t, err, "should return an immediate error if workers fail on startup") } func TestEnv(t *testing.T) { diff --git a/internal/backoff/backoff.go b/internal/backoff/backoff.go deleted file mode 100644 index 73182f4a37..0000000000 --- a/internal/backoff/backoff.go +++ /dev/null @@ -1,59 +0,0 @@ -package backoff - -import "C" -import ( - "sync" - "time" -) - -type ExponentialBackoff struct { - backoff time.Duration - failureCount int - mu sync.RWMutex - MaxBackoff time.Duration - MinBackoff time.Duration - MaxConsecutiveFailures int -} - -// recordSuccess resets the backoff and failureCount -func (e *ExponentialBackoff) RecordSuccess() { - e.mu.Lock() - e.failureCount = 0 - e.backoff = e.MinBackoff - e.mu.Unlock() -} - -// recordFailure increments the failure count and increases the backoff, it returns true if MaxConsecutiveFailures has been reached -func (e *ExponentialBackoff) RecordFailure() bool { - e.mu.Lock() - e.failureCount += 1 - if e.backoff < e.MinBackoff { - e.backoff = e.MinBackoff - } - - e.backoff = min(e.backoff*2, e.MaxBackoff) - - e.mu.Unlock() - return e.MaxConsecutiveFailures != -1 && e.failureCount >= e.MaxConsecutiveFailures -} - -// wait sleeps for the backoff duration if failureCount is non-zero. -// NOTE: this is not tested and should be kept 'obviously correct' (i.e., simple) -func (e *ExponentialBackoff) Wait() { - e.mu.RLock() - if e.failureCount == 0 { - e.mu.RUnlock() - - return - } - e.mu.RUnlock() - - time.Sleep(e.backoff) -} - -func (e *ExponentialBackoff) FailureCount() int { - e.mu.RLock() - defer e.mu.RUnlock() - - return e.failureCount -} diff --git a/internal/backoff/backoff_test.go b/internal/backoff/backoff_test.go deleted file mode 100644 index b82efc4bd4..0000000000 --- a/internal/backoff/backoff_test.go +++ /dev/null @@ -1,41 +0,0 @@ -package backoff - -import ( - "github.com/stretchr/testify/assert" - "testing" - "time" -) - -func TestExponentialBackoff_Reset(t *testing.T) { - e := &ExponentialBackoff{ - MaxBackoff: 5 * time.Second, - MinBackoff: 500 * time.Millisecond, - MaxConsecutiveFailures: 3, - } - - assert.False(t, e.RecordFailure()) - assert.False(t, e.RecordFailure()) - e.RecordSuccess() - - e.mu.RLock() - defer e.mu.RUnlock() - assert.Equal(t, 0, e.failureCount, "expected failureCount to be reset to 0") - assert.Equal(t, e.backoff, e.MinBackoff, "expected backoff to be reset to MinBackoff") -} - -func TestExponentialBackoff_Trigger(t *testing.T) { - e := &ExponentialBackoff{ - MaxBackoff: 500 * 3 * time.Millisecond, - MinBackoff: 500 * time.Millisecond, - MaxConsecutiveFailures: 3, - } - - assert.False(t, e.RecordFailure()) - assert.False(t, e.RecordFailure()) - assert.True(t, e.RecordFailure()) - - e.mu.RLock() - defer e.mu.RUnlock() - assert.Equal(t, e.failureCount, e.MaxConsecutiveFailures, "expected failureCount to be MaxConsecutiveFailures") - assert.Equal(t, e.backoff, e.MaxBackoff, "expected backoff to be MaxBackoff") -} diff --git a/phpmainthread_test.go b/phpmainthread_test.go index 81d94c2d35..e6cafe0647 100644 --- a/phpmainthread_test.go +++ b/phpmainthread_test.go @@ -175,9 +175,9 @@ func TestFinishBootingAWorkerScript(t *testing.T) { func TestReturnAnErrorIf2WorkersHaveTheSameFileName(t *testing.T) { workers = []*worker{} - w, err1 := newWorker(workerOpt{fileName: "filename.php", maxConsecutiveFailures: defaultMaxConsecutiveFailures}) + w, err1 := newWorker(workerOpt{fileName: "filename.php"}) workers = append(workers, w) - _, err2 := newWorker(workerOpt{fileName: "filename.php", maxConsecutiveFailures: defaultMaxConsecutiveFailures}) + _, err2 := newWorker(workerOpt{fileName: "filename.php"}) assert.NoError(t, err1) assert.Error(t, err2, "two workers cannot have the same filename") @@ -185,9 +185,9 @@ func TestReturnAnErrorIf2WorkersHaveTheSameFileName(t *testing.T) { func TestReturnAnErrorIf2ModuleWorkersHaveTheSameName(t *testing.T) { workers = []*worker{} - w, err1 := newWorker(workerOpt{fileName: "filename.php", name: "workername", maxConsecutiveFailures: defaultMaxConsecutiveFailures}) + w, err1 := newWorker(workerOpt{fileName: "filename.php", name: "workername"}) workers = append(workers, w) - _, err2 := newWorker(workerOpt{fileName: "filename2.php", name: "workername", maxConsecutiveFailures: defaultMaxConsecutiveFailures}) + _, err2 := newWorker(workerOpt{fileName: "filename2.php", name: "workername"}) assert.NoError(t, err1) assert.Error(t, err2, "two workers cannot have the same name") @@ -198,9 +198,8 @@ func getDummyWorker(fileName string) *worker { workers = []*worker{} } worker, _ := newWorker(workerOpt{ - fileName: testDataPath + "/" + fileName, - num: 1, - maxConsecutiveFailures: defaultMaxConsecutiveFailures, + fileName: testDataPath + "/" + fileName, + num: 1, }) workers = append(workers, worker) return worker diff --git a/testdata/failing-worker.php b/testdata/failing-worker.php index 108d2ff865..0bb001f1ed 100644 --- a/testdata/failing-worker.php +++ b/testdata/failing-worker.php @@ -1,18 +1,7 @@ = 0 && startupFailChan != nil && !watcherIsEnabled && handler.failureCount >= worker.maxConsecutiveFailures { + select { + case startupFailChan <- fmt.Errorf("worker failure: script %s has not reached frankenphp_handle_request()", worker.fileName): + handler.thread.state.Set(state.ShuttingDown) + return } - logger.LogAttrs(ctx, slog.LevelWarn, "many consecutive worker failures", slog.String("worker", worker.name), slog.Int("thread", handler.thread.threadIndex), slog.Int("failures", handler.backoff.FailureCount())) } + + if watcherIsEnabled { + // worker script has probably failed due to script changes while watcher is enabled + logger.LogAttrs(ctx, slog.LevelWarn, "(watcher enabled) worker script has not reached frankenphp_handle_request()", slog.String("worker", worker.name), slog.Int("thread", handler.thread.threadIndex)) + } else { + // rare case where worker script has failed on a restart during normal operation + // this can happen if startup success depends on external resources + logger.LogAttrs(ctx, slog.LevelWarn, "worker script has failed on restart", slog.String("worker", worker.name), slog.Int("thread", handler.thread.threadIndex)) + } + + // wait a bit and try again (exponential backoff) + backoffDuration := time.Duration(handler.failureCount*handler.failureCount*100) * time.Millisecond + if backoffDuration > time.Second { + backoffDuration = time.Second + } + handler.failureCount++ + time.Sleep(backoffDuration) } // waitForWorkerRequest is called during frankenphp_handle_request in the php worker script. @@ -171,6 +177,7 @@ func (handler *workerThread) waitForWorkerRequest() (bool, any) { // Clear the first dummy request created to initialize the worker if handler.isBootingScript { handler.isBootingScript = false + handler.failureCount = 0 if !C.frankenphp_shutdown_dummy_request() { panic("Not in CGI context") } diff --git a/worker.go b/worker.go index 96ba45e338..13e49111c2 100644 --- a/worker.go +++ b/worker.go @@ -31,41 +31,52 @@ type worker struct { var ( workers []*worker watcherIsEnabled bool + startupFailChan chan (error) ) func initWorkers(opt []workerOpt) error { workers = make([]*worker, 0, len(opt)) - workersReady := sync.WaitGroup{} directoriesToWatch := getDirectoriesToWatch(opt) watcherIsEnabled = len(directoriesToWatch) > 0 + totalThreadsToStart := 0 for _, o := range opt { w, err := newWorker(o) if err != nil { return err } + totalThreadsToStart += w.num workers = append(workers, w) } + startupFailChan = make(chan error, totalThreadsToStart) + var workersReady sync.WaitGroup for _, w := range workers { - workersReady.Add(w.num) for i := 0; i < w.num; i++ { thread := getInactivePHPThread() convertToWorkerThread(thread, w) - go func() { - thread.state.WaitFor(state.Ready) - workersReady.Done() - }() + workersReady.Go(func() { + thread.state.WaitFor(state.Ready, state.ShuttingDown, state.Done) + }) } } workersReady.Wait() + select { + case err := <-startupFailChan: + // at least 1 worker has failed, shut down and return an error + Shutdown() + return fmt.Errorf("failed to initialize workers: %w", err) + default: + // all workers started successfully + startupFailChan = nil + } + if !watcherIsEnabled { return nil } - watcherIsEnabled = true if err := watcher.InitWatcher(directoriesToWatch, RestartWorkers, logger); err != nil { return err } From 42b2ffacf40b4865c6288f6323480a9f45fbd6e5 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Thu, 13 Nov 2025 23:45:40 +0100 Subject: [PATCH 13/31] changes log to the documented version. --- threadworker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/threadworker.go b/threadworker.go index 1fba6f2402..f4ac1e1580 100644 --- a/threadworker.go +++ b/threadworker.go @@ -142,7 +142,7 @@ func tearDownWorkerScript(handler *workerThread, exitStatus int) { if worker.maxConsecutiveFailures >= 0 && startupFailChan != nil && !watcherIsEnabled && handler.failureCount >= worker.maxConsecutiveFailures { select { - case startupFailChan <- fmt.Errorf("worker failure: script %s has not reached frankenphp_handle_request()", worker.fileName): + case startupFailChan <- fmt.Errorf("too many consecutive failures: worker %s has not reached frankenphp_handle_request()", worker.fileName): handler.thread.state.Set(state.ShuttingDown) return } From 26e1408f54043c24ae0f50c42fcb3e4214c8f2f4 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Fri, 14 Nov 2025 22:06:04 +0100 Subject: [PATCH 14/31] go linting. --- threadworker.go | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/threadworker.go b/threadworker.go index f4ac1e1580..4cf64351cb 100644 --- a/threadworker.go +++ b/threadworker.go @@ -141,11 +141,9 @@ func tearDownWorkerScript(handler *workerThread, exitStatus int) { } if worker.maxConsecutiveFailures >= 0 && startupFailChan != nil && !watcherIsEnabled && handler.failureCount >= worker.maxConsecutiveFailures { - select { - case startupFailChan <- fmt.Errorf("too many consecutive failures: worker %s has not reached frankenphp_handle_request()", worker.fileName): - handler.thread.state.Set(state.ShuttingDown) - return - } + startupFailChan <- fmt.Errorf("too many consecutive failures: worker %s has not reached frankenphp_handle_request()", worker.fileName) + handler.thread.state.Set(state.ShuttingDown) + return } if watcherIsEnabled { From bcd482a643f7f6b3db254cd97f614886973470b1 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Fri, 21 Nov 2025 23:13:15 +0100 Subject: [PATCH 15/31] Resolve merge conflicts. --- phpthread.go | 4 ++-- threadregular.go | 4 ++-- threadworker.go | 7 ++++--- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/phpthread.go b/phpthread.go index f701a4a451..1726cf9d18 100644 --- a/phpthread.go +++ b/phpthread.go @@ -46,7 +46,7 @@ func newPHPThread(threadIndex int) *phpThread { func (thread *phpThread) boot() { // thread must be in reserved state to boot if !thread.state.CompareAndSwap(state.Reserved, state.Booting) && !thread.state.CompareAndSwap(state.BootRequested, state.Booting) { - panic("thread is not in reserved state: " + thread.state.name()) + panic("thread is not in reserved state: " + thread.state.Name()) } // boot threads as inactive @@ -84,7 +84,7 @@ func (thread *phpThread) shutdown() { func (thread *phpThread) setHandler(handler threadHandler) { thread.handlerMu.Lock() defer thread.handlerMu.Unlock() - if !thread.state.requestSafeStateChange(stateTransitionRequested) { + if !thread.state.RequestSafeStateChange(state.TransitionRequested) { // no state change allowed == shutdown or done return } diff --git a/threadregular.go b/threadregular.go index 09889f5d11..e65e0302aa 100644 --- a/threadregular.go +++ b/threadregular.go @@ -13,7 +13,7 @@ import ( type regularThread struct { contextHolder - state *state.ThreadState + state *state.ThreadState thread *phpThread } @@ -53,7 +53,7 @@ func (handler *regularThread) beforeScriptExecution() string { return "" } - panic("unexpected state: " + handler.state.name()) + panic("unexpected state: " + handler.state.Name()) } func (handler *regularThread) afterScriptExecution(_ int) { diff --git a/threadworker.go b/threadworker.go index 2a182adb10..d6f119a353 100644 --- a/threadworker.go +++ b/threadworker.go @@ -4,6 +4,7 @@ package frankenphp import "C" import ( "context" + "fmt" "log/slog" "path/filepath" "time" @@ -16,7 +17,7 @@ import ( // executes the PHP worker script in a loop // implements the threadHandler interface type workerThread struct { - state *state.ThreadState + state *state.ThreadState thread *phpThread worker *worker dummyFrankenPHPContext *frankenPHPContext @@ -24,7 +25,7 @@ type workerThread struct { workerFrankenPHPContext *frankenPHPContext workerContext context.Context isBootingScript bool // true if the worker has not reached frankenphp_handle_request yet - failureCount int // number of consecutive startup failures + failureCount int // number of consecutive startup failures } func convertToWorkerThread(thread *phpThread, worker *worker) { @@ -178,7 +179,7 @@ func tearDownWorkerScript(handler *workerThread, exitStatus int) { // rare case where worker script has failed on a restart during normal operation // this can happen if startup success depends on external resources if globalLogger.Enabled(globalCtx, slog.LevelWarn) { - globalLogger.LogAttrs(globalCtx, slog.LevelWarn, "worker script has failed on restart", slog.String("worker", worker.name), slog.Int("thread", handler.thread.threadIndex), slog.Int("failures", handler.failureCount)) + globalLogger.LogAttrs(globalCtx, slog.LevelWarn, "worker script has failed on restart", slog.String("worker", worker.name), slog.Int("thread", handler.thread.threadIndex), slog.Int("failures", handler.failureCount)) } } From c4e6605a582a9b4d75167dd3d22e94ea56cf4b8a Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Sun, 30 Nov 2025 23:44:25 +0100 Subject: [PATCH 16/31] better success checks. --- frankenphp.c | 14 +++++++++----- frankenphp_test.go | 2 +- testdata/env/test-env.php | 9 +++++++++ 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/frankenphp.c b/frankenphp.c index 1af3eaa2c8..c6c606d330 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -307,7 +307,7 @@ PHP_FUNCTION(frankenphp_putenv) { } // cut the string at the first '=' - char *eq_pos = strchr(setting, '='); + char *eq_pos = memchr(setting, '=', setting_len); bool put_env_success = true; if (eq_pos != NULL) { size_t name_len = eq_pos - setting; @@ -315,13 +315,17 @@ PHP_FUNCTION(frankenphp_putenv) { (setting_len > name_len + 1) ? (setting_len - name_len - 1) : 0; put_env_success = go_putenv(setting, (int)name_len, eq_pos + 1, (int)value_len); - zval val = {0}; - ZVAL_STRINGL(&val, eq_pos + 1, value_len); - zend_hash_str_update(sandboxed_env, setting, name_len, &val); + if (put_env_success){ + zval val = {0}; + ZVAL_STRINGL(&val, eq_pos + 1, value_len); + zend_hash_str_update(sandboxed_env, setting, name_len, &val); + } } else { // no '=' found, delete the variable put_env_success = go_putenv(setting, (int)setting_len, NULL, 0); - zend_hash_str_del(sandboxed_env, setting, setting_len); + if (put_env_success){ + zend_hash_str_del(sandboxed_env, setting, setting_len); + } } RETURN_BOOL(put_env_success); diff --git a/frankenphp_test.go b/frankenphp_test.go index d642fdedf6..035a196ef2 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -643,7 +643,7 @@ func testEnv(t *testing.T, opts *testOptions) { stdoutStderr, err := cmd.CombinedOutput() if err != nil { // php is not installed or other issue, use the hardcoded output below: - stdoutStderr = []byte("Set MY_VAR successfully.\nMY_VAR = HelloWorld\nUnset MY_VAR successfully.\nMY_VAR is unset.\nMY_VAR set to empty successfully.\nMY_VAR = \nUnset NON_EXISTING_VAR successfully.\n") + stdoutStderr = []byte("Set MY_VAR successfully.\nMY_VAR = HelloWorld\nUnset MY_VAR successfully.\nMY_VAR is unset.\nMY_VAR set to empty successfully.\nMY_VAR = \nUnset NON_EXISTING_VAR successfully.\nSuccessfully failed inserting wrong format, value is empty\n") } assert.Equal(t, string(stdoutStderr), body) diff --git a/testdata/env/test-env.php b/testdata/env/test-env.php index b9d374c81c..961cb19e94 100644 --- a/testdata/env/test-env.php +++ b/testdata/env/test-env.php @@ -48,5 +48,14 @@ echo "Failed to unset NON_EXISTING_VAR.\n"; } + // insert a wrongly formatted env var (should fail) + $result = putenv('wrong-format=value'); + if ($result) { + echo "Insertion successful (should not happen).\n"; + } else { + $emptyValue = getenv('wrong-format') === false ? 'empty' : 'not empty'; + echo "Successfully failed inserting wrong format, value is $emptyValue.\n"; + } + getenv(); }; From 2e1811c47ea0b3166f38b182a5bd0206961809b7 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Tue, 2 Dec 2025 22:23:40 +0100 Subject: [PATCH 17/31] Merge conflict fix. --- internal/phpheaders/phpheaders_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/phpheaders/phpheaders_test.go b/internal/phpheaders/phpheaders_test.go index a741ec38fc..5382c1ed9c 100644 --- a/internal/phpheaders/phpheaders_test.go +++ b/internal/phpheaders/phpheaders_test.go @@ -12,7 +12,7 @@ func TestAllCommonHeadersAreCorrect(t *testing.T) { for header, phpHeader := range CommonRequestHeaders { // verify that common and uncommon headers return the same result - expectedPHPHeader := GetUnCommonHeader(header) + expectedPHPHeader := GetUnCommonHeader(t.Context(), header) assert.Equal(t, phpHeader+"\x00", expectedPHPHeader, "header is not well formed: "+phpHeader) // net/http will capitalize lowercase headers, verify that headers are capitalized From 0e5964d0c6bec8730505ab3ccbfe6c89167f7d8f Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Tue, 2 Dec 2025 23:10:38 +0100 Subject: [PATCH 18/31] go fmt --- phpmainthread.go | 10 +++++----- phpthread.go | 12 ++++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/phpmainthread.go b/phpmainthread.go index 9eef023b9b..af44aaa3d2 100644 --- a/phpmainthread.go +++ b/phpmainthread.go @@ -39,11 +39,11 @@ var ( // and reserves a fixed number of possible PHP threads func initPHPThreads(numThreads int, numMaxThreads int, phpIni map[string]string) (*phpMainThread, error) { mainThread = &phpMainThread{ - state: state.NewThreadState(), - done: make(chan struct{}), - numThreads: numThreads, - maxThreads: numMaxThreads, - phpIni: phpIni, + state: state.NewThreadState(), + done: make(chan struct{}), + numThreads: numThreads, + maxThreads: numMaxThreads, + phpIni: phpIni, } // initialize the first thread diff --git a/phpthread.go b/phpthread.go index c82bf4e2c0..7fc5a515c3 100644 --- a/phpthread.go +++ b/phpthread.go @@ -16,12 +16,12 @@ import ( // identified by the index in the phpThreads slice type phpThread struct { runtime.Pinner - threadIndex int - requestChan chan contextHolder - drainChan chan struct{} - handlerMu sync.Mutex - handler threadHandler - state *state.ThreadState + threadIndex int + requestChan chan contextHolder + drainChan chan struct{} + handlerMu sync.Mutex + handler threadHandler + state *state.ThreadState } // interface that defines how the callbacks from the C thread should be handled From 1d52c10c3cf37e798ad4fdbbf12e3091bcbe3c6d Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Tue, 2 Dec 2025 23:14:00 +0100 Subject: [PATCH 19/31] go fmt --- phpmainthread.go | 10 +++++----- phpthread.go | 12 ++++++------ 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/phpmainthread.go b/phpmainthread.go index 9eef023b9b..af44aaa3d2 100644 --- a/phpmainthread.go +++ b/phpmainthread.go @@ -39,11 +39,11 @@ var ( // and reserves a fixed number of possible PHP threads func initPHPThreads(numThreads int, numMaxThreads int, phpIni map[string]string) (*phpMainThread, error) { mainThread = &phpMainThread{ - state: state.NewThreadState(), - done: make(chan struct{}), - numThreads: numThreads, - maxThreads: numMaxThreads, - phpIni: phpIni, + state: state.NewThreadState(), + done: make(chan struct{}), + numThreads: numThreads, + maxThreads: numMaxThreads, + phpIni: phpIni, } // initialize the first thread diff --git a/phpthread.go b/phpthread.go index c82bf4e2c0..7fc5a515c3 100644 --- a/phpthread.go +++ b/phpthread.go @@ -16,12 +16,12 @@ import ( // identified by the index in the phpThreads slice type phpThread struct { runtime.Pinner - threadIndex int - requestChan chan contextHolder - drainChan chan struct{} - handlerMu sync.Mutex - handler threadHandler - state *state.ThreadState + threadIndex int + requestChan chan contextHolder + drainChan chan struct{} + handlerMu sync.Mutex + handler threadHandler + state *state.ThreadState } // interface that defines how the callbacks from the C thread should be handled From dde790696430f60cad71f335fc54f8c049e18221 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Wed, 3 Dec 2025 00:13:10 +0100 Subject: [PATCH 20/31] Fixes tests. --- frankenphp.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/frankenphp.c b/frankenphp.c index 835e15adb2..d0dbee8f49 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -80,10 +80,6 @@ void frankenphp_update_local_thread_context(bool is_worker) { is_worker_thread = is_worker; } -void frankenphp_update_local_thread_context(bool is_worker) { - is_worker_thread = is_worker; -} - static void frankenphp_update_request_context() { /* the server context is stored on the go side, still SG(server_context) needs * to not be NULL */ From 45a86af54c037a2de293a28438b31df81a3ffc9e Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Thu, 4 Dec 2025 21:56:35 +0100 Subject: [PATCH 21/31] clang-format --- frankenphp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frankenphp.c b/frankenphp.c index d0dbee8f49..507bc7aec7 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -318,7 +318,7 @@ PHP_FUNCTION(frankenphp_putenv) { (setting_len > name_len + 1) ? (setting_len - name_len - 1) : 0; put_env_success = go_putenv(setting, (int)name_len, eq_pos + 1, (int)value_len); - if (put_env_success){ + if (put_env_success) { zval val = {0}; ZVAL_STRINGL(&val, eq_pos + 1, value_len); zend_hash_str_update(sandboxed_env, setting, name_len, &val); From c55dc34046d05cbd47cf7404543881952bf60950 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Thu, 4 Dec 2025 21:57:36 +0100 Subject: [PATCH 22/31] makes wrong format even wronger. --- testdata/env/test-env.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/testdata/env/test-env.php b/testdata/env/test-env.php index 961cb19e94..a8441778b0 100644 --- a/testdata/env/test-env.php +++ b/testdata/env/test-env.php @@ -49,11 +49,11 @@ } // insert a wrongly formatted env var (should fail) - $result = putenv('wrong-format=value'); + $result = putenv('wrong- format=:/usr/bin::/bin;:/opt??bin'); if ($result) { echo "Insertion successful (should not happen).\n"; } else { - $emptyValue = getenv('wrong-format') === false ? 'empty' : 'not empty'; + $emptyValue = getenv('wrong- format') === false ? 'empty' : 'not empty'; echo "Successfully failed inserting wrong format, value is $emptyValue.\n"; } From 016b63d91734843867cba6bd59c2ca8faa848b3e Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Thu, 4 Dec 2025 22:58:07 +0100 Subject: [PATCH 23/31] Adds clarification. --- frankenphp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frankenphp.c b/frankenphp.c index 507bc7aec7..e12fed4e88 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -838,8 +838,9 @@ static inline void register_server_variable_filtered(const char *key, static void frankenphp_register_variables(zval *track_vars_array) { /* https://www.php.net/manual/en/reserved.variables.server.php */ - /* In CGI mode, we consider the environment to be a part of the server - * variables. + /* In CGI mode, the environment is part of the $_SERVER variables. + * $_SERVER and $_ENV should only contain values from the original + * environment, not values added though putenv */ zend_hash_copy(Z_ARR_P(track_vars_array), main_thread_env, NULL); From 981b801275c32ee27bb2aba6f52709c2a497beea Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Thu, 4 Dec 2025 23:12:17 +0100 Subject: [PATCH 24/31] Removes test again as asan/msan will make insertions succeed. --- frankenphp.c | 2 +- testdata/env/test-env.php | 9 --------- 2 files changed, 1 insertion(+), 10 deletions(-) diff --git a/frankenphp.c b/frankenphp.c index e12fed4e88..3f8da95679 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -326,7 +326,7 @@ PHP_FUNCTION(frankenphp_putenv) { } else { // no '=' found, delete the variable put_env_success = go_putenv(setting, (int)setting_len, NULL, 0); - if (put_env_success){ + if (put_env_success) { zend_hash_str_del(sandboxed_env, setting, setting_len); } } diff --git a/testdata/env/test-env.php b/testdata/env/test-env.php index a8441778b0..b9d374c81c 100644 --- a/testdata/env/test-env.php +++ b/testdata/env/test-env.php @@ -48,14 +48,5 @@ echo "Failed to unset NON_EXISTING_VAR.\n"; } - // insert a wrongly formatted env var (should fail) - $result = putenv('wrong- format=:/usr/bin::/bin;:/opt??bin'); - if ($result) { - echo "Insertion successful (should not happen).\n"; - } else { - $emptyValue = getenv('wrong- format') === false ? 'empty' : 'not empty'; - echo "Successfully failed inserting wrong format, value is $emptyValue.\n"; - } - getenv(); }; From d3c5501ce363260085b0153ae87f392f3814e7b4 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Thu, 11 Dec 2025 23:32:29 +0100 Subject: [PATCH 25/31] Adds test. --- testdata/env/test-env.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/testdata/env/test-env.php b/testdata/env/test-env.php index b9d374c81c..8833e3091a 100644 --- a/testdata/env/test-env.php +++ b/testdata/env/test-env.php @@ -13,6 +13,14 @@ echo "Failed to set MY_VAR.\n"; } + // verify putenv does not affect $_SERVER + $result = $_SERVER[$var] ?? null; + if ($result !== null) { + echo "MY_VAR is in \$_SERVER (not expected)\n"; + } else { + echo "MY_VAR not found in \$_SERVER.\n"; + } + // Unsetting the environment variable $result = putenv($var); if ($result) { From 3aa35f990354e63c52baa3466f90e37697ec7ccf Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Thu, 11 Dec 2025 23:33:07 +0100 Subject: [PATCH 26/31] Adds custom env to start of tests. --- frankenphp_test.go | 5 ++++- worker_test.go | 3 --- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/frankenphp_test.go b/frankenphp_test.go index 7ef3a096f5..14cff6ce18 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -147,6 +147,9 @@ func TestMain(m *testing.M) { slog.SetDefault(slog.New(slog.DiscardHandler)) } + // setup custom environment var for TestWorkerHasOSEnvironmentVariableInSERVER + os.Setenv("CUSTOM_OS_ENV_VARIABLE", "custom_env_variable_value") + os.Exit(m.Run()) } @@ -645,7 +648,7 @@ func testEnv(t *testing.T, opts *testOptions) { stdoutStderr, err := cmd.CombinedOutput() if err != nil { // php is not installed or other issue, use the hardcoded output below: - stdoutStderr = []byte("Set MY_VAR successfully.\nMY_VAR = HelloWorld\nUnset MY_VAR successfully.\nMY_VAR is unset.\nMY_VAR set to empty successfully.\nMY_VAR = \nUnset NON_EXISTING_VAR successfully.\nSuccessfully failed inserting wrong format, value is empty\n") + stdoutStderr = []byte("Set MY_VAR successfully.\nMY_VAR = HelloWorld\nUnset MY_VAR successfully.\nMY_VAR is unset.\nMY_VAR set to empty successfully.\nMY_VAR = \nUnset NON_EXISTING_VAR successfully.\n") } assert.Equal(t, string(stdoutStderr), body) diff --git a/worker_test.go b/worker_test.go index fe17d2c422..65e019538e 100644 --- a/worker_test.go +++ b/worker_test.go @@ -8,7 +8,6 @@ import ( "net/http" "net/http/httptest" "net/url" - "os" "strconv" "strings" "testing" @@ -148,8 +147,6 @@ func ExampleServeHTTP_workers() { } func TestWorkerHasOSEnvironmentVariableInSERVER(t *testing.T) { - assert.NoError(t, os.Setenv("CUSTOM_OS_ENV_VARIABLE", "custom_env_variable_value")) - runTest(t, func(handler func(http.ResponseWriter, *http.Request), _ *httptest.Server, i int) { req := httptest.NewRequest("GET", "http://example.com/worker.php", nil) w := httptest.NewRecorder() From c803d7c8772713ca01be59cbe7812dc5900cb211 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Thu, 11 Dec 2025 23:33:29 +0100 Subject: [PATCH 27/31] Never clears main thread env. --- env.go | 4 ++-- frankenphp.c | 22 ++++++++-------------- 2 files changed, 10 insertions(+), 16 deletions(-) diff --git a/env.go b/env.go index 79639fb860..e99da53f3d 100644 --- a/env.go +++ b/env.go @@ -12,7 +12,7 @@ import ( ) //export go_init_os_env -func go_init_os_env(trackVarsArray *C.HashTable) { +func go_init_os_env(mainThreadEnv *C.zend_array) { env := os.Environ() for _, envVar := range env { key, val, _ := strings.Cut(envVar, "=") @@ -22,7 +22,7 @@ func go_init_os_env(trackVarsArray *C.HashTable) { var zval C.zval *(*uint32)(unsafe.Pointer(&zval.u1)) = C.IS_INTERNED_STRING_EX *(**C.zend_string)(unsafe.Pointer(&zval.value)) = zvalStr - C.zend_hash_update(trackVarsArray, zkey, &zval) + C.zend_hash_update(mainThreadEnv, zkey, &zval) } } diff --git a/frankenphp.c b/frankenphp.c index 3f8da95679..46b451a555 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -607,15 +607,6 @@ static zend_module_entry frankenphp_module = { TOSTRING(FRANKENPHP_VERSION), STANDARD_MODULE_PROPERTIES}; -static void frankenphp_request_shutdown() { - if (sandboxed_env != NULL) { - zend_hash_release(sandboxed_env); - sandboxed_env = NULL; - } - frankenphp_free_request_context(); - php_request_shutdown((void *)0); -} - static int frankenphp_startup(sapi_module_struct *sapi_module) { php_import_environment_variables = get_full_env; @@ -1015,10 +1006,6 @@ static void *php_main(void *arg) { frankenphp_sapi_module.ini_entries = NULL; } - /* free the main thread environment, necessary for tests */ - zend_hash_release(main_thread_env); - main_thread_env = NULL; - go_frankenphp_shutdown_main_thread(); return NULL; @@ -1079,7 +1066,14 @@ int frankenphp_execute_script(char *file_name) { zend_destroy_file_handle(&file_handle); - frankenphp_request_shutdown(); + /* Reset env varibales added through putenv() */ + if (sandboxed_env != NULL) { + zend_hash_release(sandboxed_env); + sandboxed_env = NULL; + } + + frankenphp_free_request_context(); + php_request_shutdown((void *)0); return status; } From 3fc0d92a6bcbed24587addc39e16281079853313 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Thu, 11 Dec 2025 23:43:26 +0100 Subject: [PATCH 28/31] Adds putenv syntax check. --- frankenphp.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/frankenphp.c b/frankenphp.c index 46b451a555..dc89ed7e29 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -305,6 +305,11 @@ PHP_FUNCTION(frankenphp_putenv) { RETURN_FALSE; } + if (setting_len == 0 || setting[0] == '=') { + zend_argument_value_error(1, "must have a valid syntax"); + RETURN_THROWS(); + } + if (sandboxed_env == NULL) { sandboxed_env = zend_array_dup(main_thread_env); } @@ -1066,7 +1071,7 @@ int frankenphp_execute_script(char *file_name) { zend_destroy_file_handle(&file_handle); - /* Reset env varibales added through putenv() */ + /* Reset values added through putenv() */ if (sandboxed_env != NULL) { zend_hash_release(sandboxed_env); sandboxed_env = NULL; From e00a70eb54d07f899db0acea198cc27c3abf5c12 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Fri, 12 Dec 2025 00:06:22 +0100 Subject: [PATCH 29/31] Adds more tests. --- env.go | 2 +- frankenphp_test.go | 2 +- testdata/env/test-env.php | 9 +++++++++ 3 files changed, 11 insertions(+), 2 deletions(-) diff --git a/env.go b/env.go index e99da53f3d..f7bcd0df4c 100644 --- a/env.go +++ b/env.go @@ -31,7 +31,7 @@ func go_putenv(name *C.char, nameLen C.int, val *C.char, valLen C.int) C.bool { goName := C.GoStringN(name, nameLen) if val == nil { - // Unset the environment variable + // If no "=" is present in putenv(...), unset the environment variable return C.bool(os.Unsetenv(goName) == nil) } diff --git a/frankenphp_test.go b/frankenphp_test.go index 14cff6ce18..154ebaab02 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -648,7 +648,7 @@ func testEnv(t *testing.T, opts *testOptions) { stdoutStderr, err := cmd.CombinedOutput() if err != nil { // php is not installed or other issue, use the hardcoded output below: - stdoutStderr = []byte("Set MY_VAR successfully.\nMY_VAR = HelloWorld\nUnset MY_VAR successfully.\nMY_VAR is unset.\nMY_VAR set to empty successfully.\nMY_VAR = \nUnset NON_EXISTING_VAR successfully.\n") + stdoutStderr = []byte("Set MY_VAR successfully.\nMY_VAR = HelloWorld\nMY_VAR not found in $_SERVER.\nUnset MY_VAR successfully.\nMY_VAR is unset.\nMY_VAR set to empty successfully.\nMY_VAR = \nUnset NON_EXISTING_VAR successfully.\nInvalid value was not inserted.\n") } assert.Equal(t, string(stdoutStderr), body) diff --git a/testdata/env/test-env.php b/testdata/env/test-env.php index 8833e3091a..85181e127c 100644 --- a/testdata/env/test-env.php +++ b/testdata/env/test-env.php @@ -56,5 +56,14 @@ echo "Failed to unset NON_EXISTING_VAR.\n"; } + // Inserting an invalid variable should fail (null byte in key) + putenv("INVALID\x0_VAR=value"); + $value = getenv("INVALID\x0_VAR"); + if ($value) { + echo "Invalid value was inserted (unexpected).\n"; + } else { + echo "Invalid value was not inserted.\n"; + } + getenv(); }; From c578745f3ed48355f7ab00d088598705d1e3a129 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Fri, 12 Dec 2025 13:50:20 +0100 Subject: [PATCH 30/31] Fixes os env tests. --- caddy/caddy_test.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/caddy/caddy_test.go b/caddy/caddy_test.go index 7cf69bfa5e..812db7e842 100644 --- a/caddy/caddy_test.go +++ b/caddy/caddy_test.go @@ -21,6 +21,14 @@ import ( var testPort = "9080" +func TestMain(m *testing.M) { + // setup custom environment vars for TestOsEnv + os.Setenv("ENV1", "value1") + os.Setenv("ENV2", "value2") + + os.Exit(m.Run()) +} + func TestPHP(t *testing.T) { var wg sync.WaitGroup tester := caddytest.NewTester(t) @@ -904,8 +912,6 @@ func testSingleIniConfiguration(tester *caddytest.Tester, key string, value stri } func TestOsEnv(t *testing.T) { - os.Setenv("ENV1", "value1") - os.Setenv("ENV2", "value2") tester := caddytest.NewTester(t) tester.InitServer(` { From 75a07f8834c847cbdc3871173bd94b4955e2e572 Mon Sep 17 00:00:00 2001 From: Alliballibaba Date: Fri, 12 Dec 2025 14:03:28 +0100 Subject: [PATCH 31/31] cleanup. --- caddy/caddy_test.go | 6 ++++-- env.go | 5 ++--- frankenphp.c | 2 +- frankenphp_test.go | 5 ++++- testdata/env/test-env.php | 3 +-- 5 files changed, 12 insertions(+), 9 deletions(-) diff --git a/caddy/caddy_test.go b/caddy/caddy_test.go index 812db7e842..96ea7eb50c 100644 --- a/caddy/caddy_test.go +++ b/caddy/caddy_test.go @@ -23,8 +23,10 @@ var testPort = "9080" func TestMain(m *testing.M) { // setup custom environment vars for TestOsEnv - os.Setenv("ENV1", "value1") - os.Setenv("ENV2", "value2") + if os.Setenv("ENV1", "value1") != nil || os.Setenv("ENV2", "value2") != nil { + fmt.Println("Failed to set environment variables for tests") + os.Exit(1) + } os.Exit(m.Run()) } diff --git a/env.go b/env.go index f7bcd0df4c..31963ebd3e 100644 --- a/env.go +++ b/env.go @@ -13,8 +13,7 @@ import ( //export go_init_os_env func go_init_os_env(mainThreadEnv *C.zend_array) { - env := os.Environ() - for _, envVar := range env { + for _, envVar := range os.Environ() { key, val, _ := strings.Cut(envVar, "=") zkey := C.frankenphp_init_persistent_string(toUnsafeChar(key), C.size_t(len(key))) zvalStr := C.frankenphp_init_persistent_string(toUnsafeChar(val), C.size_t(len(val))) @@ -31,7 +30,7 @@ func go_putenv(name *C.char, nameLen C.int, val *C.char, valLen C.int) C.bool { goName := C.GoStringN(name, nameLen) if val == nil { - // If no "=" is present in putenv(...), unset the environment variable + // If no "=" is present, unset the environment variable return C.bool(os.Unsetenv(goName) == nil) } diff --git a/frankenphp.c b/frankenphp.c index dc89ed7e29..6a3fa36075 100644 --- a/frankenphp.c +++ b/frankenphp.c @@ -1071,7 +1071,7 @@ int frankenphp_execute_script(char *file_name) { zend_destroy_file_handle(&file_handle); - /* Reset values added through putenv() */ + /* Reset values the sandboxed environment */ if (sandboxed_env != NULL) { zend_hash_release(sandboxed_env); sandboxed_env = NULL; diff --git a/frankenphp_test.go b/frankenphp_test.go index 154ebaab02..07b9590bb1 100644 --- a/frankenphp_test.go +++ b/frankenphp_test.go @@ -148,7 +148,10 @@ func TestMain(m *testing.M) { } // setup custom environment var for TestWorkerHasOSEnvironmentVariableInSERVER - os.Setenv("CUSTOM_OS_ENV_VARIABLE", "custom_env_variable_value") + if os.Setenv("CUSTOM_OS_ENV_VARIABLE", "custom_env_variable_value") != nil { + fmt.Println("Failed to set environment variable for tests") + os.Exit(1) + } os.Exit(m.Run()) } diff --git a/testdata/env/test-env.php b/testdata/env/test-env.php index 85181e127c..002a45df02 100644 --- a/testdata/env/test-env.php +++ b/testdata/env/test-env.php @@ -58,8 +58,7 @@ // Inserting an invalid variable should fail (null byte in key) putenv("INVALID\x0_VAR=value"); - $value = getenv("INVALID\x0_VAR"); - if ($value) { + if (getenv("INVALID\x0_VAR")) { echo "Invalid value was inserted (unexpected).\n"; } else { echo "Invalid value was not inserted.\n";