brefphp / symfony-bridge

Bref runtime to run Symfony on AWS Lambda
MIT License
47 stars 14 forks source link

When using brief runtime directly , KnpPaginatorBundle sorting keep the parameters from the 1st 'cold' request #59

Open allan-simon opened 2 years ago

allan-simon commented 2 years ago

i.e in pages like employees?direction=desc&page=1&sort=e.lastName the sort and direction stay to the one of the cold request, i.e if you do quickly a second request that change this, the new value value will ignored

if you wait until the lambda is recycled and a new one is done, the parameter works

switching from php-fpm layer to direct BrefRuntime trigger/fix the issue

allan-simon commented 2 years ago

to be more precise , it seems to be a side effect of applying the work around for https://github.com/brefphp/symfony-bridge/issues/58 , so we currently either have this issue , or the #58 ^^'

mnapoli commented 2 years ago

It seems to be the same thing as #58

allan-simon commented 2 years ago

sorry, i made some mistake, the two are not related , I was just mixing two branches and thought the fix of one was triggering the other. So Knp alone does not work too. so yes it's a case of "this bundle is not compatible with keeping the process alive"

However that means that we already have 2 major bundle not compatible, i.e not the kind of bundle that resort to "bad practice" coding. So I think it is interesting for me to find why they don't work, and on which assumption they are built, which is no more true in the 'multi invocation per process' context. Identifying this may help having a general set of rules to apply for people to make their bundle "100% lambda compatible"

t-richard commented 2 years ago

This is a common problem when executing multiple requests within the same process. State in services will be reused across requests.

Most of those bundles weren't thought with this in mind and would not be compatible with swoole, Roadrunner, ReactPHP and similar either.

Symfony provides a reset interface that can fill this gap but the most obvious way to handle this is avoid storing state inside services.

If you identify the culprit service, you could decorate the services and make it implement the interface.

You could also try to override the service definition from the bundle to mark the service as shared: false which means Symfony DI will create a new instance each time the service is requested. Not sure if that would work to be honest but might be worth a try.

allan-simon commented 2 years ago

@t-richard thanks for the tips, yes as I was digging yesterday, for the case of Knp, it seems to be because there are some event subscriber register by a Service from KNP that are constructed with the request of that time. My understanding is that as then this service is no more called (because as you said the service is already created as far as symfony is concerned) then the subscribers stay for ever with the 1st request

I'm going to see if it's possible to instead give them the request stack, so that they can do "getCurrentRequest()" and get the actual current request.

t-richard commented 2 years ago

I think a kernel.request subscriber would be called on each request and not only on the first one.

My bet is that the subscriber is caching the value inside a private property or something similar to avoid recomputing things several times inside the same request.

Not sure if that's the subscriber you are referring to but in this one there is some state stored in $this->params.

This is probably never cleared which would cause sub-sequent requests to use the same values. To test this hypothesis, you could decorate the service (see this doc) and make your decorator implement the ResetInterface then reset the fields (like params). If it works, it may be worth a PR/issue on their repo.

NOTE: this is me looking quickly at a code I don't know, I may be totally wrong on the source of your issue :sweat_smile:

allan-simon commented 2 years ago

@t-richard thanks for the pointers, that's the direction i'm heading in