Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(frankenphp-symfony): add kernel reboot strategy #166

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

phramz
Copy link

@phramz phramz commented Jan 19, 2024

I was facing weird side effects regarding PHP sessions and Doctrine ORM running Symfony 6.x and 7.x applications in FrankenPHP worker-mode.

Rebooting the kernel between requests fixed it.

I had this kind of issues back than using Roadrunner as well, so the reboot strategy implementation is inspired by https://github.com/Baldinof/roadrunner-bundle.

Though I guess an on_exception strategy does not make to much sense on a runtime layer (since FrankenPHP will kill the worker if an exception is not handled), it's just 'never' and 'always' for now.

@Rainrider
Copy link

Hm, does this not offset the benefits of the worker mode?

@alexander-schranz
Copy link
Member

@phramz that sounds like an issue that some services may forgot to implement the KernelResetInterface. In the worker mode the kernel should be keep bootet and only services resetted.

@phramz
Copy link
Author

phramz commented Mar 6, 2024

@Rainrider Since the reboot takes place between requests, it will take some extra time until the worker is able the handle the next request which might impact performance in case all workers are busy (e.g. under high load). This should not impact other "benefits" of the worker mode.

The kernel reboot mode defaults to never. Where always is just a plan B so one is able to run applications shipping bundles/libs that introduce side-effects by not cleaning up their global states between requests.
Since many developers assumed PHP will handle clean up and libs were not developed having long running process execution in mind.

@alexander-schranz which KernelResetInterface are you referring to?

@alexander-schranz
Copy link
Member

alexander-schranz commented Mar 6, 2024

The https://github.com/symfony/symfony/blob/7.1/src/Symfony/Contracts/Service/ResetInterface.php (service tag kernel.reset) is responsible that a Symfony Service clean it self up without have to reboot the Kernel. If you have problems with session it may an issue with the sessions you may access the session differently then over the Symfony objects? Symfony itself does there the cleanup of the session in its Listeners.

@phramz
Copy link
Author

phramz commented Mar 6, 2024

@alexander-schranz I guess that might be the general root cause for the issues.
I had this type of side effects in two different SF/Doctrine projects so far using RoadRunner as well as FrankenPHP in worker mode recently.

For me it pretty much looks related to Baldinof/roadrunner-bundle#116

Unfortunately I didn't have enough time to do perform an in the depth debugging of all components being involved, so I went for a kernel reboot between requests (luckily RR supported it) that solved it for me.

@Rainrider
Copy link

@phramz is there any benefit left other than not having to pay start-up costs for the worker itself? Please excuse my lack of knowledge in this regard.

Depending on your answer it might be better to just disable worker mode instead.

@phramz
Copy link
Author

phramz commented Mar 8, 2024

@Rainrider The biggest advantage of using worker-mode is the performance gain. This is due to request hit a warm (running) application. That eliminates expensive overhead like parsing, class loading, etcpp.
The trade-off is that this introduces a global application state prone to side-effects between requests that needs to be addressed by all components by let's say cleaning up after themselves (at the latest) when the request/response cycle is finished.

@alexander-schranz
Copy link
Member

alexander-schranz commented Mar 8, 2024

That eliminates expensive overhead like parsing, class loading, etcpp.

Class loading and parsing is what the opcache is in PHP for and you can optimize that part via opcache.preload. The worker mode really advantages come in place wehre the Kernel is kept booted and services instanced and only need to reset their states instead of rebuilding all services again. I think there is not much difference if you use reboot after every request and not using the worker mode. Maybe reboot requires even some more resources as it need to destroy more instead of killing all directly.

The easiest way to find where you need to add a ResetInterface is to search for something like \$this\-\>(.*) = which is not inside a constructor. Services should in best practices be stateless (readonly). That is also why in frameworks like Symfony Request object are not longer services like in old versions.

Keep in mind if you use example async messenge:consume via Symfony Messenger you need to take care of the same things via the ResetInterface, to make sure the services are between 2 consumed messages correctly reseted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants