fix(worker): reset ini and session if changed during worker request

[xavierleune]

Hi there,

I was investigating the bug reported in #977 and I found that there is a lot of things that leaks between worker requests:

• all changes to ini values
• session handlers (that was the origin of the bug thrown in /app/vendor/symfony/http-foundation/Session/Storage/NativeSessionStorage.php #977 as the session_handler is destroyed but the configuration still references it)

The current pull requests now:

• Checks all values of ini & session before the first request is handled
• Reset values that changed between requests

This changes improves support of worker mode for legacy applications, that do nasty stuff sometimes ^^

[AlliBalliBaba]

Not sure if the session logic belongs in this repo. On one hand we reload the session module, which purges all sessions and handlers. On the other hand this PR would then go back in and re-instate the session handler, which feels very messy.
I think it would make more sense to either not reload native sessions at all between requests and let the user-space handle this (same as with other globals). Or to add a hook into the session extension itself to make it compatible with a stateful model.

Restoring ini entries seems more sensible o me 👍 . We'd have to consider though that restoring ini entries can have other side-effects. Say, memory_limit is already above a certain value and you're trying to restore it to a lower value, that would obviously fail. Would be nice knowing what else could potentially fail.

[henderkes]

Restoring ini entries seems more sensible o me 👍 . We'd have to consider though that restoring ini entries can have other side-effects. Say, memory_limit is already above a certain value and you're trying to restore it to a lower value, that would obviously fail. Would be nice knowing what else could potentially fail.

I agree, but I don't see a good way to handle this on the FrankenPHP side of things, as we'd need to wait for a gc cycle before attempting this in any case. If a request increases the memory limit "temporarily" it's typically for good reason. Giving user land the option to "save state" and "reroll state" would make it a simple adjustment in worker scripts, but that's not the cleanest design.

[xavierleune]

Not sure if the session logic belongs in this repo. On one hand we reload the session module, which purges all sessions and handlers. On the other hand this PR would then go back in and re-instate the session handler, which feels very messy.

Actually I think it's critical to have worker requests to reset state for that kind of things to whatever it was before the first request was handled. This is the root cause of the "invalid callback" bug.
It may be a good idea to supports this in the core of PHP, but in my opinion we should not wait for that kind of changes: Frankenphp aims to offer worker mode to apps from PHP 8.2, so that kind of optimization should be made only when all php versions supported by Franken offers it.
Besides this, today Frankenphp only works well with major frameworks and modern applications, I think it should have a better support of legacy apps that does weird (but supported by php) things. That PR really improves the support of legacy apps

[henderkes]

I disagree. Session handling is not part of FrankenPHP specifically. I also don't think it's realistic to have "any php app written in history needs to be compatible with worker mode" as a goal. At what point do we draw the line and stop short of completely rewriting php?

[xavierleune]

This issue is caused by the module reloading of the session, as it destroys handlers that have been set by the request, without removing the references to those handlers. I think it's an inconsistent behavior and should be treated as a bug.

[AlliBalliBaba]

It might be a bug in the context of Symfony 5, not sure if other applications currently rely on it being flushed tough. Since the handler is a custom class, it also can have private properties, which you might not want to bleed across requests.

Not sure what the ideal solution is, just saying that this is easier to do on the PHP side by just re-registering the handler.

[xavierleune]

I understand it might be better from the php side. However I didn't had this issue in a symfony app.
I think it would be better if we want worker mode to be more broadly used to find a workeraround in frankenphp. We can also have a better solution on php for an upcoming version and rely on it conditionally

[dunglas]

Worker scripts are usually provided by frameworks. I would prefer to keep the worker code as small and focused as possible.
It's totally expected that the PHP code has to be slightly adapted for worker mode.

IMHO, we should document these cases explicitly instead of adding code specially for legacy apps. It will never be possible cover all possible leaks directly in the worker mode anyway.

[xavierleune]

@dunglas I may agree on ini, but sessions should be fixed on frankenphp level. I don't think php offers a userland api to restore the session handler correctyle and it's directly caused by the reload of the module. WDYT ?

[dunglas]

I agree for sessions

[henderkes]

If we're already doing sessions, which I really see more on php's side, we must definitely do it for ini settings too.

[AlliBalliBaba]

I don't think php offers a userland api to restore the session handler

Can't you just re-register the handler right before session_start ?

[xavierleune]

@AlliBalliBaba actually that's trickier, you don't need to use the session in the request 2 to have a crash. Let's assume the following workflow:

1. Worker starts
2. First request uses session and defines a session handler
3. Session handlers will be destroyed but reference retained at the end of the execution
4. 2nd request comes in, doesn't need session (other user, other path, and so), you have a crash at the end of this request because of the handler you cannot "undefine" at the end of request 1.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The first call to erealloc() when entries_to_restore is NULL should use emalloc() instead, as erealloc(NULL, size) behavior may not be portable. Initialize entries_to_restore with emalloc() on the first allocation.

Suggested change

	restore_capacity = restore_capacity ? restore_capacity * 2 : 8;
	entries_to_restore = erealloc(entries_to_restore,
	restore_capacity * sizeof(zend_string *));
	if (restore_capacity == 0) {
	restore_capacity = 8;
	entries_to_restore =
	emalloc(restore_capacity * sizeof(zend_string *));
	} else {
	restore_capacity *= 2;
	entries_to_restore =
	erealloc(entries_to_restore,
	restore_capacity * sizeof(zend_string *));
	}

[xavierleune]

Hey there, may an expert provide some guidance about copilot feedbacks? I'm not used to C so I'm not sure if I should blindly trust the suggestions or not.
Thanks for your help!