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

[2.3] [WIP] Synchronized services... #7007

Merged
merged 9 commits into from Mar 23, 2013

Conversation

Projects
None yet
@fabpot
Member

fabpot commented Feb 7, 2013

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #5300, #6756
License MIT
Doc PR symfony/symfony-docs#2343

Todo:

  • update documentation
  • find a better name than contagious (synchronized)?

refs #6932, refs #5012

This PR is a proof of concept that tries to find a solution for some problems we have with scopes and services depending on scoped services (mostly the request service in Symfony).

Basically, whenever you want to inject the Request into a service, you have two possibilities:

  • put your own service into the request scope (a new service will be created whenever a sub-request is run, and the service is not available outside the request scope);
  • set the request service reference as non-strict (your service is always available but the request you have depends on when the service is created the first time).

This PR addresses this issue by allowing to use the second option but you service still always has the right Request service (see below for a longer explanation on how it works).

There is another issue that this PR fixes: edge cases and weird behaviors. There are several bug reports about some weird behaviors, and most of the time, this is related to the sub-requests. That's because the Request is injected into several Symfony objects without being updated correctly when leaving the request scope. Let me explain that: when a listener for instance needs the Request object, it can listen to the kernel.request event and store the request somewhere. So, whenever you enter a sub-request, the listener will get the new one. But when the sub-request ends, the listener has no way to know that it needs to reset the request to the master one. In practice, that's not really an issue, but let me show you an example of this issue in practice:

  • You have a controller that is called with the English locale;
  • The controller (probably via a template) renders a sub-request that uses the French locale;
  • After the rendering, and from the controller, you try to generate a URL. Which locale the router will use? Yes, the French locale, which is wrong.

To fix these issues, this PR introduces a new notion in the DIC: synchronized services. When a service is marked as synchronized, all method calls involving this service will be called each time this service is set. When in a scope, methods are also called to restore the previous version of the service when the scope leaves.

If you have a look at the router or the locale listener, you will see that there is now a setRequest method that will called whenever the request service changes (because the Container::set() method is called or because the service is changed by a scope change).

@lsmith77

This comment has been minimized.

Contributor

lsmith77 commented Feb 7, 2013

will review the PR in more detail when I get home but I think proxy objects might be a better solution. we could support automatically "unloading" of predict loaded proxies referencing request scoped services when we enter/leave the given sub request.

@fabpot

This comment has been minimized.

Member

fabpot commented Feb 7, 2013

This PR is transparent as far as the services are concerned, which is one of the primary characteristics of the service container, the objects must not know that they are managed by the service container. How would proxy objects work with listeners?

@lsmith77

This comment has been minimized.

Contributor

lsmith77 commented Feb 7, 2013

As the DIC is the one that creates the proxies, it would it could track them and therefore ensure that the state is managed.

So when I access the request proxy for the first time in a service it would fetch the request service from the DIC instance inside the proxy and so the proxy would become initialized. Then when I leave the scope/enter a new scope the DIC can automatically revert the back to the uninitialized state, ensuring that the service would be initialized again based on the now current state of the DIC.

@alvarezmario

This comment has been minimized.

Contributor

alvarezmario commented Feb 7, 2013

@fabpot By the way, the example you put about the locales, is currently an issue in the 2.2 branch, but not in the 2.1.
I switch back to 2.1 just because of this.

@fabpot

This comment has been minimized.

Member

fabpot commented Feb 7, 2013

On Twitter, someone suggested volatile instead of contagious?

@fabpot

This comment has been minimized.

Member

fabpot commented Feb 7, 2013

@lsmith77: Have a look at the PR, my solution is much simpler than that, no moving parts.

@lsmith77

This comment has been minimized.

Contributor

lsmith77 commented Feb 7, 2013

Well I dont think my proposal is anymore complicated, especially of we add support for proxied services anyway.

My concern is mostly about "propagating" or "recalling" the setters, because in theory the service could have assigned a request scoped service to another dependency and it may not be practical to have to write logic in the setter to then propagate this as well.

Now in my proposal instead of recalling the setters, you simply iterate over the proxies and reset their state. this will then also update any dependency that are not managed by the DIC.

from a users perspective its more reliable and i think from the DIC point of view its not any more complex either. again assuming we want to add proxy support anyway.

@sstok

This comment has been minimized.

Contributor

sstok commented Feb 7, 2013

Is using array_key_exists() really needed here? I know isset() is (was??) faster then array_key_exists().
And I'm pretty sure using null service names is not allowed.

About the naming +1 for volatile.

@lsmith77

This comment has been minimized.

Contributor

lsmith77 commented Feb 7, 2013

volatile indeed sounds better .. does Spring not have similar needs?

@jorgelbg

This comment has been minimized.

jorgelbg commented Feb 7, 2013

I think that the volatile name does not make justice, yes the service will be volatile changing with each request, but it will be fixed to the request itself, so it will not be available outside the scope of the request which will kind of confusing?.

@fabpot

This comment has been minimized.

Member

fabpot commented Feb 7, 2013

On Twitter still, someone suggests "synchronized"

@merk

This comment has been minimized.

Contributor

merk commented Feb 7, 2013

Looks excellent.

May I suggest Propagate, given the name is already in use in the code?

@fabpot

This comment has been minimized.

Member

fabpot commented Feb 7, 2013

@merk: isSynchronized() sounds good to me but isPropagatable() sounds weird, no?

@merk

This comment has been minimized.

Contributor

merk commented Feb 7, 2013

You're right. It sounds weird.

The correct way of saying it is isPropagable -- https://en.wiktionary.org/wiki/propagable

Another suggestion: isInfectious() 😏

@marcospassos

This comment has been minimized.

marcospassos commented Feb 7, 2013

Synchronized sounds good! 👍

@marcospassos

This comment has been minimized.

marcospassos commented Feb 7, 2013

I think that @lsmith77 solution may solve the issue #6985.

@lsmith77

This comment has been minimized.

Contributor

lsmith77 commented Feb 8, 2013

how about "scopeSafe"?

@drm

This comment has been minimized.

Contributor

drm commented Feb 8, 2013

I suggested synchronized on twitter, because for some reason I couldn't comment here with my phone...

The reason I think it is a good name is because it implies that the container does some work for you.

(edit:) And volatile has an "unstable" ring to it, but that's of course a matter of taste.

@jorgelbg

This comment has been minimized.

jorgelbg commented Feb 8, 2013

What about "Scoped Services" for the new concept?

On 2/8/13, Gerard van Helden notifications@github.com wrote:

I suggested synchronized on twitter, because for some reason I couldn't
comment here with my phone...

The reason I think it is a good name is because it implies that the
container does some work for you.


Reply to this email directly or view it on GitHub:
#7007 (comment)

@fabpot

This comment has been minimized.

Member

fabpot commented Feb 9, 2013

I've just updated the code to use synchronized instead of contagious.

@fabpot fabpot referenced this pull request Feb 9, 2013

Closed

added triggers #64

@igorw

View changes

src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php Outdated
@@ -557,7 +557,7 @@ protected function get{$name}Service()
*/
private function addServices()
{
$publicServices = $privateServices = $aliasServices = '';
$publicServices = $privateServices = $aliasServices = $propagaters = '';

This comment has been minimized.

@igorw

igorw Feb 9, 2013

Contributor

propagators? s/e/o

This comment has been minimized.

@fabpot

fabpot Feb 9, 2013

Member

I've renamed it to synchronizers as this is more accurate now.

}
}
private function callMethod($service, $call)

This comment has been minimized.

@benja-M-1

benja-M-1 Feb 10, 2013

Contributor

The PHPDoc is missing here

This comment has been minimized.

@jameshalsall

jameshalsall Feb 11, 2013

Contributor

I don't think private methods require a PHPDoc block in the symfony2 coding standards, no?

This comment has been minimized.

@Crell

Crell Feb 12, 2013

Contributor

IMO all methods should have a PHPDoc block, regardless of their scope. Someone is going to read this code and wonder what those variables are supposed to be.

This comment has been minimized.

@jameshalsall

jameshalsall Feb 12, 2013

Contributor

@Crell I agree whole-heartedly, but I didn't think private methods needed them (think I'm wrong though according to the standards mentioned here -- http://symfony.com/doc/current/contributing/code/standards.html#documentation)

@stof

This comment has been minimized.

Member

stof commented Feb 11, 2013

@nomack84 if you want the issue in 2.1, consider the case of using the Intl locale after the subrequest, and you face it too (see my report in #6932)

@@ -206,6 +206,10 @@ public function set($id, $service, $scope = self::SCOPE_CONTAINER)
}
$this->services[$id] = $service;
if (method_exists($this, $method = 'synchronize'.strtr($id, array('_' => '', '.' => '_')).'Service')) {

This comment has been minimized.

@Crell

Crell Feb 12, 2013

Contributor

Mixing the assignment into the method_exists() call like that makes the code quite hard to read. Can we break it out to a separate $method = line and then the if() statement? That would improve readability considerably.

*
* @param string $id A service id
*/
private function synchronize($id)

This comment has been minimized.

@Crell

Crell Feb 12, 2013

Contributor

This method feels like it could get quite slow. Once you have a decent number of services initialized it's going to result in a lot of iterations, even if only a scant few of them result in a method call. Could we add a tracking index of some kind to keep track of what methods would need to get called on what services?

This comment has been minimized.

@Crell

Crell Feb 12, 2013

Contributor

Hm, or maybe the PhpDumper is handling that? I don't understand the dumper code well enough to understand if it is. If the dumped code is more optimized than this and would not iterate over all services every time a synchronized service changed, that's good enough, I think. If it would still use this method, then this method should be optimized.

This comment has been minimized.

@fabpot

fabpot Feb 12, 2013

Member

That's the case, this method is not called when using a dumped container (a dumper container extends Container and not ContainerBuilder). Here is an example of a dumped code (see the unit tests below):

    protected function synchronizeRequestService()
    {
        if ($this->initialized('depends_on_request')) {
            $this->get('depends_on_request')->setRequest($this->get('request', ContainerInterface::NULL_ON_INVALID_REFERENCE));
        }
    }

And for services without the syncrhonized flag, the method does not even exist.

@Crell

This comment has been minimized.

Contributor

Crell commented Feb 12, 2013

Overall I think this looks like a solid approach, and would resolve the issues we ran into in Drupal. It should be fairly easy for us (and anyone else using the DI component) to adapt to it. Nice work, Fabien!

"synchronized" sounds like a fine name to me.

I think this works better than proxy objects per @lsmith's suggestion, since it avoids new object instantiation entirely. It looks like existing constructor-based dependencies would still reinitialize, though, which gives developers a choice of whether or not it makes sense to reinitialize their services.

@sstok

This comment has been minimized.

Contributor

sstok commented Apr 14, 2013

A little off topic, but if we abolish scopes completely. How would we handle scope="prototype" then?

Really like to idea of @beberlei !

And it should be possible to introduce this without BC breaks no?

@beberlei

This comment has been minimized.

Contributor

beberlei commented Apr 14, 2013

@sstok my idea would only be about removing scope="request" (in the long run, i.e. 3.0)

@lsmith77

This comment has been minimized.

Contributor

lsmith77 commented Apr 14, 2013

i would like to propose that we hold an IRC meeting to discuss this .. ideally tomorrow evening since we do not have much time until the end of the dev phase for 2.3

@ghost

This comment has been minimized.

ghost commented Apr 14, 2013

I will attend if it happens.

@beberlei

This comment has been minimized.

Contributor

beberlei commented Apr 14, 2013

I am afraid i am not into the topic deep enough to be able to responsibly say something about this. In the beta phase we maintained the rule that only people with insight into the code are allowed to vote on changes. I am not sure that except @fabpot anyone (maybe @stof ) can reasonable say that he can make an informed decision about this topic.

My impression was that synchronized services is only necessary, because the request is wrongly a service, although its a data/value object. I proposed an idea about how to possibly solve this, and slowly deprecate request as a service.

This was done without thinking about consequences like:

  1. How to maintain BC
  2. How to adapt the current listeners to it
  3. How a good solution would look like and if my proposed solution is really the best one.

"request" as a service is a very fundamental concept in the MVC stack of Symfony2, it may well be that this pushes us to necessary solutions that we could have avoided much earlier, but not anymore in the 2.* lifecycle.

I know Fabien tried to work with the request stack prior before merging this PR, so his decision to merge this took all the three points into account, which i cannot say from my experiments.

I don't think it makes sense to discuss this topic further without having a concrete alternative on the table in form of a pull request. Since Fabien tried several different approaches here, I am not sure that any option is not something he has considered already.

So without his definitive input on the topic with regard to the possibility of an alternative solutions, it doesn't make sense to discuss it.

@webmozart

This comment has been minimized.

Contributor

webmozart commented Apr 14, 2013

I agree with @igorw's blog post that this PR is problematic. More than being a blurry concept, "synchronized" has a very different connotation for any developer that used Java before (in Java, "synchronized" marks exclusively accessible code/services in multi-threaded environments).

I also agree with @beberlei that the request should not be a service. It is no "service" by definition, as it does not process any business logic. It's just a data container.

To me, a "requestContext" service with a getCurrentRequest() method seems like a simple and totally suitable solution.

@fabpot

This comment has been minimized.

Member

fabpot commented Apr 14, 2013

It might seem obvious today that the Request is not a service, but back in the days, I fought hard to avoid the introduction of the Response as a service as well. So, we only have half of a problem today ;)

The Request object is indeed a very special "service", or put another way, this is the only data object that is/should be managed by the service container. Why? to have the ability to inject the Request into other services.

But just to be able to get the Request from the DIC, we had to introduce a lot of different notions: the request scope, synchronized services, strict arguments, synthetic services, and perhaps more. Even if all those concepts might be of interest by themselves, the truth is that they are not that often useful when managing "regular" services (and Silex/Pimple is probably the best demonstration of that).

But as of this PR, I think we now have a working model for managing the Request and its life-cycle in the context of the DIC. I say "working model" because an average developer can now depend on the Request without knowing anything about the internals.

That being said, I'm the first to recognize that things are complex behind the scene just for this one object (even if this is probably the most important one). I also agree that having so many features just to be able to easily work with the Request is a strong smell that something is probably wrong.

So, what about @beberlei proposal? Here are my first thoughts:

  • It seems that it would fix a lot of our problems quite nicely and easily, but we would loose one thing that we have as of 2.3: the ability to inject the Request like any other service (except the possibility to inject it in a constructor which seems like a very small drawback).
  • I tried this approach when I was working on this PR and I hit a wall quite fast. But IIRC, it was because I didn't want to inject a request registry instead of the real Request.
  • Backward compatibility is a big concern here. Not that introducing a request registry would break BC, but we would probably have to keep everything we already have (including the synchronized services) for the foreseeable future anyway. So the question is if it makes sense to maintain two ways of doing things.
  • Related to the previous point, this new way will need to be documented along side the current way. We will need to make it easy to grasp and not confusing for people used to the old way. That should probably be not too difficult though.

As many of you probably know now, I'm not that good at thinking/talking about concepts without real/working code. So, I will play with @beberlei code in the next couple of days and get back here with my findings and thoughts (and I highly encourage others to do the same to get familiar with it). Then, we will list the pros and the cons of both approaches and that will allow us to discuss what to do for 2.3 and/or 3.0.

Enough for now, let's go back to the code now!

@webmozart

This comment has been minimized.

Contributor

webmozart commented Apr 14, 2013

What do you think about reverting the PR until a final decision is made?

@igorw

This comment has been minimized.

Contributor

igorw commented Apr 14, 2013

A note about BC in relation to @beberlei's proposal: It might be possible to create a ProxyRequest which extends the Request, overrides all public methods to delegate to an internally stored request registry. There is some cost in terms of complexity/performance, but it could be done.

@fabpot

This comment has been minimized.

Member

fabpot commented Apr 15, 2013

@bschussek Whatever the decision we take, we won't revert this PR as it fixes bugs with our current approach. And as we would have to keep the current way for BC reasons, this bug fix should stay in anyway. That's one of the reasons I did not consider using a registry/proxy in the first place when I was working on a solution for the problems we had.

@igorw We discussed that already (it was @lsmith77 proposal IIRC) and that's out of question: the request is used quite frequently, so we won't introduce a proxy for such an important object in Symfony.

Another drawback for @beberlei proposal I've just thought about:

  • As of 2.3, a service depending on the request (and doing it the right way) does not need to depend on the event dispatcher anymore, which obviously won't be the case with the registry. And that's an important property: a service depending on the request does not need to care about the machinery we have; it should be reusable outside of the context of Symfony.
@lsmith77

This comment has been minimized.

Contributor

lsmith77 commented Apr 15, 2013

When passing the request via action method signatures, we could pass in the plain, unproxied, request instance. There is no reason to ever put a controller into the request scope, same applies to listeners, that I guess also are guaranteed to get the proper request instance via the event object?

@fabpot

This comment has been minimized.

Member

fabpot commented Apr 15, 2013

@lsmith77 You are right for controllers, but for listeners, the problem is a bit different: they get the right request when the scope is entered, but before this PR, they were no aware that a request scope finished. And this is the only problem that this PR tried to address.

@lsmith77

This comment has been minimized.

Contributor

lsmith77 commented Apr 15, 2013

Just FYI .. we already have one place where we use a registry which is the ManagerRegistry for doctrine.

@henrikbjorn

This comment has been minimized.

Contributor

henrikbjorn commented Apr 15, 2013

@lsmith77 which is also a pain point when using the bridges outside of the full stack framework as everything in components relies on a manager and the only implementation of it i have seen is the one in symfony.

@stof

This comment has been minimized.

Member

stof commented Apr 19, 2013

@lsmith77 the goal of the ManagerRegistry is to allow multiple managers in the app (and retrieved lazily), not to stack managers on top of each other. So we don't have the issue of poping the old one.

@stof

This comment has been minimized.

@fabpot This broke the implementation of Doctrine\Common\Persistence\AbstractManagerRegistry::resetService in Symfony\Bridge\Doctrine\Registry.
This class is setting the service to null, relying on the fact that it was triggering the instantiation of a new entity manager in Symfony 2.2 and below when calling get again later. Now, calling getManager after resetting the manager in the registry is returning null, which breaks the code (as the method should never return null) and thus makes the resetting unusable.

With the new logic, I don't see any way to fix the registry implementation, and this is clearly an undocumented BC break in 2.3.

This comment has been minimized.

Contributor

beberlei replied Jun 24, 2013

we could check for setting null in set and then unset the id

This comment has been minimized.

Member

stof replied Jun 24, 2013

well, the question is whether switching from isset to array_key_exists was needed to implement the feature

@samuelvogel

This comment has been minimized.

Contributor

samuelvogel commented Jul 23, 2013

What actually is the status of this issue? The fix discussed here is marked as merged, but #6932 is still open/broken. It's a little hard to comprehend this long discussion without background knowledge!

@fabpot fabpot referenced this pull request Aug 31, 2013

Merged

Synchronized Service alternative, backwards compatible #8904

5 of 5 tasks complete

fabpot added a commit that referenced this pull request Aug 15, 2016

minor #19608 [DependencyInjection] ContainerBuilder: Remove obsolete …
…definitions (ogizanagi)

This PR was merged into the 3.1 branch.

Discussion
----------

[DependencyInjection] ContainerBuilder: Remove obsolete definitions

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

IIUC, [the obsolete definitions thing was tied to scoped & sync services](#7007).
Thus, this code [is not meant to be used since 3.0 and can be removed](#13289).

However, it may be considered as a BC break and would require introducing a new deprecation instead, because the current code allows to set a service on a frozen container if it has an obsolete synthetic definition...which is weird, isn't it ? (I doubt this feature was explicitly intended to allow setting a synthetic service more than once, and it wasn't before sync services introduction)

I suggest this as a patch for 3.1, under the pretext of forgotten code tied to a previous deprecation & feature removal, but let me know if it should be considered differently.

Commits
-------

daa7d00 [DependencyInjection] ContainerBuilder: Remove obsolete definitions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment