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

Twig doesn't support alternative PHP servers like RoadRunner #4007

Open
speller opened this issue Mar 10, 2024 · 10 comments
Open

Twig doesn't support alternative PHP servers like RoadRunner #4007

speller opened this issue Mar 10, 2024 · 10 comments

Comments

@speller
Copy link

speller commented Mar 10, 2024

Alternative PHP servers like RoadRunner don't unload PHP processes and process multiple requests per process allowing to get a significant performance increase. Unfortunately, Twig is not fully compatible with such servers because it relies on stateful classes that store some data. The intention is good - to cache data during the request. However, if the PHP process is not unloaded between HTTP requests, the internal state becomes an issue. On the second and further requests, Twig gets data from the first one which easily can be broken and make the request fail.

From my investigation, I understood that the issue is mostly in the global variables. They're cached in the \Twig\Environment::$resolvedGlobals property. Some extensions like EasyAdmin may add global variables conditionally. For example, if the first request doesn't meet EasyAdmin's criteria, the ea global variable is not loaded and not added to $resolvedGlobals. On the next request, Twig sees the non-null $resolvedGlobals and doesn't try to reload global variables. So any subsequent request that tries to render EasyAdmin's stuff will fail because the ea variable is not loaded.

Currently, to fix this issue, I'm using the following workaround in my project:

Define a custom Twig class:

class Twig extends Environment implements EventSubscriberInterface
{
    public function onKernelTerminate(TerminateEvent $event): void
    {
        // Clear the resolvedGlobals cache to reinitialize global variables on the next RR request
        $this->resolvedGlobals = null;
    }

    #[\Override]
    public static function getSubscribedEvents(): array
    {
        return [
            KernelEvents::TERMINATE => 'onKernelTerminate',
        ];
    }
}

Add a compiler pass to replace the Twig object with the custom one:

class ReplaceTwigCompilerPass implements CompilerPassInterface
{

    #[\Override]
    public function process(ContainerBuilder $container): void
    {
        $twigDefinition = $container->getDefinition('twig');
        $twigDefinition->setClass(Twig::class);
        $twigDefinition->addTag('kernel.event_subscriber');
    }
}

The custom class subscribes to Symfony's kernel.terminate event and cleans up globals cache. So on the next request, Twig properly reinitializes it with variables for the current request.

And the last point - to allow the Environment's $resolvedGlobals field modification, I apply the following patch with cweagans/composer-patches:

--- a/src/Environment.php
+++ b/src/Environment.php
@@ -57,7 +57,7 @@
     private $compiler;
     /** @var array<string, mixed> */
     private $globals = [];
-    private $resolvedGlobals;
+    protected $resolvedGlobals;
     private $loadedTemplates;
     private $strictVariables;
     private $templateClassPrefix = '__TwigTemplate_';

For my project, this works well. But I would be more than happy to see native support to not apply custom workarounds. The fix doesn't look complex so I believe it can be implemented quickly with no breaking changes.

@GromNaN
Copy link
Contributor

GromNaN commented Mar 15, 2024

Symfony has the ResetInterface::reset() that can be called between requests. This is something that could be implemented in Twig. This means that we iterate on every extension to get the globals that may be defined by them. But I believe all Twig configuration should be static, including the config provided by extensions.

The underlying issue is having a global variable defined conditionally in EasyAdmin. Instead it could be injected as parameter when templates are rendered (in one place in AbstractCrudController::render()); or the ea global variable always defined with a proxy object.

@speller
Copy link
Author

speller commented Mar 18, 2024

Proxy object for the ea variable is definitely an option. But I think it will be a workaround. The root issue here in Twig itself, it should be able to handle multiple requests without script unload by design.

@GromNaN
Copy link
Contributor

GromNaN commented Mar 18, 2024

Twig is perfectly able to handle multiple request: it can render several templates with the same Twig\Environment instance; the environment is idempotent. It's because there's a condition in the EasyAdmin extension that there's a problem. It's like initializing 2 different Twig environments depending on whether it is in an EasyAdmin context or not.

@speller
Copy link
Author

speller commented Mar 19, 2024

@GromNaN Twig allows EasyAdmin to do this. So this is a Twig issue as well. The current case with EasyAdmin can be resolved by introducing a workaround in it. But any other Twig extension can fall into the same issue. This is why I suggest solving the root cause instead of relying on specific extensions to follow some unwritten agreement of not doing something.

@speller
Copy link
Author

speller commented Mar 19, 2024

@GromNaN While I agree that Symfony has some tools to react to new requests in the same app, Twig is abstract from frameworks and, from my point of view, should support resetting its environment between requests. So then the Symfony bundle will be able to call this public Twig API with no hacks, workarounds, or extensions rewriting and solve the root cause for all cases, not for EasyAdmin specifically.

@GromNaN
Copy link
Contributor

GromNaN commented Mar 19, 2024

Resetting the environment is almost the same as reinstantiating the Twig\Environment class. The definition of what should be reset or not vague. That would lead to more unexpected behaviors and performance issues.

Note that even if you reset the Twig environment, if you process 2 requests in parallel (co-routines with Fibers), you can end with an incorrect state for both of them.

@speller
Copy link
Author

speller commented Mar 20, 2024

@GromNaN Why do we need to reinstantiate the whole instance if we need to cleanup only global variables? Is it already allowed in the Symfony integration?

Fibers is a completely different topic and I would not mix them here.

@AlexBachmann
Copy link

I experience similar issues, when running shopware/shopware with FrankenPHP. The globals are not reset between requests, which can cause significant rendering problems in Shopware, since the globals are, among others used for URL creation, and marking of page types.

@nicolas-grekas
Copy link
Contributor

The issue is using globals for request-local state.
See EasyCorp/EasyAdminBundle#6273 for a discussion about this, and a possible solution, which might not belong to Twig in the end.

@nicolas-grekas
Copy link
Contributor

Maybe twig should deprecate GlobalsInterface?

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

No branches or pull requests

4 participants