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

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

Open
allan-simon opened this issue Feb 16, 2022 · 7 comments

Comments

@allan-simon
Copy link
Contributor

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
Copy link
Contributor Author

to be more precise , it seems to be a side effect of applying the work around for #58 , so we currently either have this issue , or the #58 ^^'

@mnapoli
Copy link
Member

mnapoli commented Feb 16, 2022

It seems to be the same thing as #58

@allan-simon
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Member

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 😅

@allan-simon
Copy link
Contributor Author

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

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

No branches or pull requests

3 participants