Skip to content

[DX] Promote phppm and add support for resettable services #23984

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

Closed
zmitic opened this issue Aug 25, 2017 · 20 comments
Closed

[DX] Promote phppm and add support for resettable services #23984

zmitic opened this issue Aug 25, 2017 · 20 comments
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature
Milestone

Comments

@zmitic
Copy link

zmitic commented Aug 25, 2017

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes
Symfony version 3.3.x

Intro

phppm is a library that makes php application run 10-15 times faster compared to ./bin console server:run. And unlike internal php web server, phppm supports concurrent requests which makes developing angular state machines much faster.

Problem

Because services are shared between requests, any memoized data can/will affect next request. Doctrine identity map is the best example but it is easy solvable; only do $em->clear() in kernel.terminate.

I am using it for about a month and so far, I spotted only 2 problems. One is data collectors but it a problem only in debug mode so not something of big relevance.

The second is EntityType service. It has very nice optimization of queries but keeps results in $choiceLoaders property. Parent class DoctrineType takes care of it, check the code.

This creates 2 problems when used in phppm enviroment, first one is described here but closed in favor of this ticket. The second problem is that memory will keep pilling up for every form with EntityType and it cannot be solved using DTO objects.

The solution

It should be pretty easy to solve it. I made a bundle that will collect all services with ResettableServiceInterface implemented and call reset() when kernel.terminate event is triggered.

So for example, if this RFC is accepted, DoctrineType would only have this extra code:

abstract class DoctrineType extends AbstractType implements ResettableServiceInterface
{
    public function reset()
    {
        $this->choiceLoaders = [];
    }
}

Any other service that does memoization can be easily upgraded to work with phppm.

Proposal

Symfony already promotes popular bundles and while phppm is not technically a bundle, I think it should be placed there. I am willing to write docs for that.

This solution should be part of core, not a third-party bundle. More people will adopt it that way because not all developers are familiar with phppm but also because it is part of core and people prefer opinionated solutions.

Technical

You guys are far more skilled than me but if you check my code and think it is good enough, I could make a PR. Please keep in mind that would be my first contribution so be gentle 😄

I would suggest one more thing, the new config value:

framework:
    phppm: false

If false, than services would not be reset to avoid instantiating them.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Aug 25, 2017

TL;DR, you propose adding a ResettableServiceInterface and call reset() on kernel.terminate.

This ResettableServiceInterface looks like a good idea to me, but not in Symfony unfortunately: a service (any class really) shouldn't know about its container. But requiring classes to implement ResettableServiceInterface would break this rule.

This might be something for PHP-FIG thought.

For the 2 issues you spotted, I think we have to find more specific solutions - just not this one...

@stof
Copy link
Member

stof commented Aug 25, 2017

the fact that this forces any package containing a memomizing class to depend on the package defining the interface is precisely what I highlighted in #23975 as major pain point to implement such an idea.

@zmitic
Copy link
Author

zmitic commented Aug 25, 2017

@stof @nicolas-grekas I got it now and you are right, it should something for PHP-FIG.

Given that Symfony is leader in PHP world, does it make sense to implement it here and when PSR is created one day, move to it like ContainerInterface was?

It would work only for bundles at beginning but I think it is a good start.

@nicolas-grekas About

For the 2 issues you spotted, I think we have to find a solution - just not this one..

Can a simple public function reset() be added to DoctrineType::class to avoid reflection?

@nicolas-grekas
Copy link
Member

public function reset() be added to DoctrineType::class

with a doctrine listener that would call it on terminate? you should give it a try in a PR yes

@derrabus
Copy link
Member

A component should not care about whether it's constructed and managed by a container or not. Also, a library like Doctrine for instance should not have an opinion on what kind of data needs to be flushed for a new request. This is why I also would oppose such an interface.

However, for the bundle that registers a component as a set of services, it's a different story. DoctrineBundle could reset the EM, FrameworkBundle could reset the form type you mentioned, WebProfilerBundle could flush data collectors that would leak memory otherwise. So if a generic cleanup mechanism would be implemented, the framework should imho hand over the responsibility to the bundles.

Bundles could register event handlers that to do the cleanup work. I could see the following approaches:

  • Each bundle registers its own listeners to the kernel.terminate event and does its cleanup there. Since cleaning up is not necessary in shared-nothing environments (which is php's default, after all), those listeners should be opt-in. The result would be that I had to turn on those cleanup tasks in each bundle's configuration. This might not be the perfect way, but it would be possible without changing the framework itself.
  • A new event kernel.reset (or similar) is added that would only be triggered in environments like phppm. This would work as with the kernel.terminate event, but the bundles don't need to take care of the opt-in to the reset anymore. They simply register their cleanup tasks on that new event.

The event handlers in both approaches have the downside that they might unnecessarily reset a service that has not been constructed yet. Alternatively, we could try to solve this with the container. The container builder would receive a mechanism to configure method calls that reset a service.

services:
    some_service:
        class: Acme\SomeService
        on_reset:
            - myResetMethod
            - ['@some_other_service', anotherResetMethod]

The container would have a reset() (or you name it) method that would iterate over all constructed services with that configuration and call the configured methods.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Aug 27, 2017

@derrabus it might be worth pursuing the generic idea (the per-bundle-listener-style has many drawbacks you spotted so it doesn't look worth it to me.)

So, about the generic one:

A new event kernel.reset

why a new event? kernel.terminate looks just fine to me, isn't it?

unnecessarily reset a service that has not been constructed yet

I would try so resist as much as possible to the need for a new config option, but the concern is valid.
We do have $container->initialized() for public services. But that leaves the issue open for private ones.

Another option would be to have a new type of service arguments (similar to IteratorArgument) that yields instances only if they are already initialized.
With such a primitive, we could then use a tag, eg. kernel.resettable, and make everything work with a compiler pass and a generic event listener: the event listener would call a reset() method on all instantiated services on kernel.terminate.

Would that make sense? Would you like to give it a try?

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Aug 27, 2017
@NicoHaase
Copy link
Contributor

Just out of curiosity: shouldn't it be enough to reset all instantiated services by setting $this->services = [] in the container?

@nicolas-grekas
Copy link
Member

In theory it could, but this wouldn't clear the static properties (if any leak comes from there). It wouldn't also clear circular references - unless an explicit call to the garbage collector is done. But planning for resetting all services + triggering the GC might defeat most of the perf benefit of php-pm-like strategies.
Having explicit cooperation from services that "leak" might be a more robust solution.

@javiereguiluz javiereguiluz added DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature labels Aug 28, 2017
@javiereguiluz
Copy link
Member

I'm not sure about this proposal, but it would be great to make Symfony 100% compatible with PHP-PM, for those devs who want to use it. In #18244 @marcj pointed out the biggest problem to make that happen.

@derrabus
Copy link
Member

@NicoHaase Yes, you could do that, but the idea of phppm is to gain speed by not throwing everything away between requests. So keeping constructed services and only deleting request-related data from them is the desirable behavior here.

One could also think about just unsetting those services that carry request-related data, but then we might get strange behavior in services that have already consumed the service we've just unset.

@nicolas-grekas For me, the question whether we solve this with a config option or a tag is the question if we make this a feature of the container itself (=> config option) or a feature that FramworkBundle adds to the component (=> tag + compiler pass).

I don't want to bloat the container, but I think this feature could be useful for projects that use the container without the framework.

why a new event? kernel.terminate looks just fine to me, isn't it?

If we don't take the approach of event handlers registered by bundles, we don't need a new event. I simply wanted to move the decision whether cleanup event handlers are necessary or not out of the bundles. After all, preparing the application for another request was not the original purpose of kernel.terminate. It just happens to be the last event in the cycle.

Another option would be to have a new type of service arguments (similar to IteratorArgument) that yields instances only if they are already initialized.

I don't see a generic use-case in such a feature. Also, the content of that iterator might change after each request. This could make its behavior unpredictable and volatile.

Would you like to give it a try?

I'll work on a prototype and see how it works out.

@nicolas-grekas
Copy link
Member

this feature could be useful for projects that use the container without the framework

if we can do it with frameworkbundle, without patching DI, it means anyone can do it also.
to me, the tag + compiler pass + event listener belong to HttpKernel, DI needs this primitive to get uninitialized services, and framework just some config to wire things together

about uninitialized services, you still need a way to (not) deal with them, how would you do it then?

@nicolas-grekas
Copy link
Member

See #24033 for uninitialized services

@Nek-
Copy link
Contributor

Nek- commented Aug 30, 2017

Hello, this post may be a little bit out of topic but it's about the part of "promoting phppm".

I don't see how phppm could replace the bin/console server:run if you consider that its power comes precisely from the fact it doesn't need to get classes on each HTTP call, which is exactly what we expect in a development environment. Does phppm support hot-reload or something like that?

About the rest: that's awesome trying to move on a real support for phppm. This is really interesting. Thanks for your work Symfony Team!!! 😄

@jeffreymb
Copy link
Contributor

jeffreymb commented Aug 30, 2017

@Nek- Yes, php-pm does support "hot reloading".

@Koc
Copy link
Contributor

Koc commented Aug 30, 2017

@Nek- see php-pm/php-pm#14

fabpot added a commit that referenced this issue Aug 31, 2017
…EFERENCE (nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Add ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

As spotted in #23984 and #18244, supporting php-pm and the likes requires us to have a way to reset services, typically on `kernel.terminate`. Yet, only actually instantiated services need to be resetted.

Knowing if a service is initialized or not is doable for public ones thanks to `Container::initialized()`. But for private ones, there is no way to achieve it.

I propose to add a new way to reference services via this new const. When a reference has this behavior, two things happens:
- at runtime, if the referenced service is not initialized, it behave the same as "ignore on invalid", pretending the service doesn't exist
- at compile time, such references are weak, meaning their referenced services *can* be removed, but *cannot* be inlined

Commits
-------

0db3358 [DI] Add ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Sep 1, 2017

@derrabus #24033 is now merged.

To sum up the plan we talked about on Slack:
in HttpKernel, we could add a compiler pass to manage a kernel.reset (or kernel.resettable) tag, taking a "method" attribute (<tag name="kernel.reset" method="reset">)
still in HttpKernel, an EventSubscriber would then be registered on kernel.terminate to call all those methods.
The list of services to reset would be passed to the event subscriber as an iterator of initialized-only services (see #24033).

Once ready, we could leverage that feature and review components per components the services that would need this resettable behavior to comply with php-pm&the-likes.

We could also add a "ResettableInterface" in HttpKernel, so that this behavior could be easily autoconfigured.

@derrabus
Copy link
Member

@nicolas-grekas Please have a look at #24155.

fabpot added a commit that referenced this issue Sep 14, 2017
…e services (derrabus)

This PR was merged into the 3.4 branch.

Discussion
----------

[FrameworkBundle][HttpKernel] Add DI tag for resettable services

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23984
| License       | MIT
| Doc PR        | TODO

This PR uses #24033 to introduce a DI tag for resettable services.

TODO after merge:
* Add an interface, make the "method" attribute optional and enable autoconfiguration.
* Consider adding a config option to enable/disable this feature.
* Configure leaking services of the core bundles as resettable.

Commits
-------

d9a6b76 A DI tag for resettable services.
nicolas-grekas added a commit that referenced this issue Sep 15, 2017
…rrabus)

This PR was merged into the 3.4 branch.

Discussion
----------

[FrameworkBundle] Reset stopwatch between requests

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23984
| License       | MIT
| Doc PR        | N/A

Follow-up to #24155. This PR ensures that the stopwatch is reset between requests.

Commits
-------

7c3bdd9 Reset stopwatch.
@nicolas-grekas
Copy link
Member

See #24232 and doctrine/DoctrineBundle#695 that should fix the DoctrineEntity-related issue.

@zmitic
Copy link
Author

zmitic commented Sep 16, 2017

Thank you guys, this kernel.reset is amazing thing and very simple to use.

nicolas-grekas added a commit that referenced this issue Sep 20, 2017
…nicolas-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[Bridge\Doctrine] Add "DoctrineType::reset()" method

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23984
| License       | MIT
| Doc PR        | -

Works with doctrine/DoctrineBundle#695

Commits
-------

dce1eb9 [Bridge\Doctrine] Add "DoctrineType::reset()" method
@nicolas-grekas
Copy link
Member

Happy to close this one, that's a great step forward :)

chalasr pushed a commit that referenced this issue Sep 24, 2017
…n requests (derrabus)

This PR was merged into the 3.4 branch.

Discussion
----------

[SecurityBundle] Reset the authentication token between requests

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23984
| License       | MIT
| Doc PR        | N/A

Follow-up to #24155. This PR resets the token storage, making sure there's no user logged in at the beginning of a new request.

Commits
-------

e46b366 Reset the authentication token between requests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony) Feature
Projects
None yet
Development

No branches or pull requests

9 participants