I would propose we either make this a global setting only or a php_server block setting only. I currently don't see why this should be a worker setting.

See #280

As I wrote: this can be dealt with on the PHP side, but this is an ops concern, so it would make sense to have this in the Caddyfile, don't you think?

Actually I had a closer look and realized we should do the same full reset on worker threads too - something userland PHP cannot do. Both worker and module mode now do full ZTS cleanup via ts_free_thread() when max_requests is reached.

That complements user-side refreshes that clean only application state.

PR description updated, patch ready and green 🚀

See #280

As I wrote: this can be dealt with on the PHP side, but this is an ops concern, so it would make sense to have this in the Caddyfile, don't you think?

I just meant that I don't think the setting should exist in more than one place in the Caddyfile config. I'm leaning towards it only being a php_server setting (which would affect all workers in that block too).

What about having only frankenphp { max_requests } that applies to everything? That would work also for global workers.

Would be fine, although I think that we generally want almost all options to be inside php_server blocks. I think workers should have never been in global scope either.

BTW, what about defaulting to eg 100?

I don't think we should. Default should stay 0 for performance and because most apps don't actually have memory issues in non-worker mode.

Done, there's not a single max_requests under frankenphp, default to 0 = unlimited.

Can we get a quick vote on frankenphp wide vs per-php_server?

👍 for frankenphp, 👎 for php_server
@php/frankenphp-collaborators

My reasoning for going with only frankenphp after your suggestion: the ini settings and list of enabled extensions is global. If there's a leak to accommodate for, it's thus global also.

Thanks for the review @henderkes, should be all addressed now. (just waiting for the CI now)

Thanks for the review @henderkes, should be all addressed now. (just waiting for the CI now)

Hmm, that's a good point.

Thanks for the thorough review! Applied all simplifications. Two items I kept:

• go_frankenphp_on_thread_shutdown check (phpthread.go): The Is(Rebooting) -> RebootReady routing is needed: reboot() goroutine calls WaitFor(RebootReady). Without the check, we'd always set Done and the goroutine would hang.

• RebootReady in threadworker.go: I added handler.state.Set(state.Ready) before handler.beforeScriptExecution(), otherwise it's infinite recursion (re-enters with same state). This ensures it falls into the Ready case which runs updateContext, onThreadReady, and setupWorkerScript.

Or did I miss something?

Not sure this needs its own section in the docs

right, removed
thanks for the review!

I've added an entry back in the doc to expand a bit on the tension this option creates.
Excerpt:

If you notice memory growing over time, the ideal fix is to report the leak to the responsible extension or library maintainer.
But when the fix depends on a third party you don't control, max_requests provides a pragmatic and hopefully temporary workaround for production:

/cc @dunglas

Code-wise, this looks good to me.

However, I'm still not entirely convinced by the use case. The question is: do we want to introduce and maintain this complexity because some (proprietary) extensions have issues or not.

Normal code shouldn't leak, and open source code can and should be patched instead of using such workarounds.

WDYT @php/frankenphp-collaborators?

I think in an optimal world, we wouldn't have to maintain this. However, in the world we're living in, Blackfire and some other profilers do still have issues and they're slow to fix them. PHP-FPM has also had this setting forever, so we wouldn't really be going out of line there.

I think an advantage of having this PR is at least having a mechanism in place to restart a regular thread. The maintenance added by this is acceptable to me.

I agree with Marc. While extension maintainers should of course fix their leaks, the addition is lightweight and it's opt-in, making it not-that-easy to discover (which is good news and should always stay as-is).

I'm really torn between the idea that "other languages don't need to restart like that, developers are careful about memory leaks and deal with it because it's part of the job" and the reality of the situation.

Even though it’s not ideal, I’m leaning toward a positive opinion on this addition.

Perhaps we could also mark the setting // EXPERIMENTAL. That way we could remove it at a later date without breaking our "BC promises".

+1 for merging this as "experimental"

I like this addition since rebooting threads has other potential use cases. But as @dunglas mentioned the downside is that it might encourage extensions not to fix their leaks. +1 for experimental

PR updated, experimental note added.

FTR, I talked with blackfire engineers: they aren't able to prioritize looking at this because frankenphp is a very very small share of their customer.s And since frankenphp isn't stable with blackfire, their customers are not going to adopt frankenphp either. Chicken and egg.

And since frankenphp isn't stable with blackfire, their customers are not going to adopt frankenphp either.

And even worse, they're not going to get any customers who care about performance, either, by not properly supporting FrankenPHP or Swoole. You can take a guess why we chose not to adopt Blackfire 😅

What are the other use case do you have in mind @AlliBalliBaba ?

I think we could also use similar logic to restart all threads when watching or on opcache_reset. Basically, keep everything stalled on the go side and just restart the C threads, might be cleaner than forcing opcache to reset and probably only minimal additional overhead.

Another side effect is that this allows configuring max_requests for workers via Caddyfile, which might be necessary for graceful restarts if we ever support async workers on the PHP side (probably far away, but theoretically already possible with amp/react).

I'm indifferent to the leak prevention though, since this has never been a problem that I have personally observed.