The Routing loader should not use resources that have dependencies #168

Closed
fabpot opened this Issue Feb 15, 2013 · 13 comments

4 participants

@fabpot
Symfony member

The AsseticLoader class (https://github.com/symfony/AsseticBundle/blob/master/Routing/AsseticLoader.php#L58) uses specific resources that have dependencies on the HttpKernel FileLocator, which in turn needs the Kernel. When Symfony serializes the resources for the routing, it serializes the kernel, which is then unserialized to see if all resources are fresh.

This is wrong as it means that for each request (in the dev environment only), Symfony needs to unserialize a new Kernel, a new FileLocator, which causes problems (see symfony/symfony#6254).

@fabpot fabpot referenced this issue in symfony/symfony Feb 15, 2013
Closed

2 issues with new fatal error displaying #6254

@stof
Symfony member

duplicate of #80

But do you have an idea how to handle the Twig templates as resources ? Knowing if a template has changed is provided by the Twig_Loader. The template may not be a file.

@mpdude

We've come across the same problem in our own codebase where we need to have "database" resources - that is, stuff that was loaded from a DB and we need to see whether it is still valid.

The problem with the current resource implementation is that it is a little bit based on the assumption that resources are files and that a last modified timestamp is all they need to check for themselves whether they are fresh.

What we did as a work-around is to have a modified implementation of ConfigCache (see Symfony PR 5912 how we would like to inject that) that holds a list of "Resource wakeups". After unserializing the resources file, all "wakeups" are passed every resource so they have a chance of re-initializing the resources. After that, resources are checked for freshness as usual. You can see that over there.

Pro: Mostly retrofittable on the current API.
Cons: Ugly. Resources have to __sleep partially. Wakeups need to have a look at Resources and figure out whether they know the implementation and know how to re-inject stuff.

Another way that I would consider much more elegant:

ConfigCache could have a list of "voters" (?). Resources become merely value objects. All voters can have a look at every Resource and decide whether it is fresh, stale or unknown (pass on to next voter).

That way, the dependencies this issue is about would be in the voter, not the serialized resource. In turn, the ConfigCache instance should probably come from the DIC (see above-mentioned PR) so that voters can be injected (probably tag-based?).

Also, ConfigCache currently behaves differently in kernel.debug and production. That could become a different set of "voters" (or different implementations) for the respective environment.

@stof
Symfony member

@mpdude Taking the config cache from the container is not possible for the one used in the kernel, as it is about caching the container (so the container is not yet available)

@mpdude

Yes, the container's cache poses a chicken-egg problem. The PR does not try to factory-create the cache there.

Maybe in this single case the config cache could be hardwired and use only a fixed set of voters (supporting only files)?

@mpdude

If there were no "concrete" routes for the assets in the Router, would the RouteCollection still need to depend on the Assetic resources at all?

I mean, the AsseticController could as well take the name and position of an asset as variable parameters and it should be possible to pass the values as such in Twig\AsseticNode instead of having dedicated routes for all assets.

Maybe that could isolate the issue in the bundle and not require support on the Symfony/Config side. Additionally, no need to re-load all the other routes when assets change (think quicker change-save-reload cycles for the frontend folks).

Or is the route cache expiration and re-load somehow used as a trigger to rebuild the assets as well?

@stof
Symfony member

@mpdude the issue is that Assetic allows you to give the output path for your asset. Your suggestion would indeed simplify the support of the AsseticController feature (by far) but would not respect the pattern anymore.

@kriswallsmith

I wonder if moving the Assetic controller to its own listener would address this issue? We would no longer be adding resources to the route collection.

@stof
Symfony member

@kriswallsmith keeping a controller is better IMO from a semantic POV. But using a single route (without respecting the output pattern of the asset) would indeed make the code far simpler.

@mpdude

Haven't checked yet... but it could be possible to make the AsseticController just take the whole pathinfo (asset target path + name/position) as parameter and only care about the name/position.

Adding the target path (as it currently happens in AsseticLoader to be part of the route) would then have to happen not where the route is loaded but where the route is used. That would be adding the path as a parameter in AsseticNode::getPathFunction()?

@stof
Symfony member

@mpdude Respecting the output pattern while using a single route would force to use a catch-all route (as the output pattern can be anything). But this will likely break the website (you probably don't want to see all urls being matched by the AsseticController)

@mpdude

Whoops, just figured that out myself while coding a POC :)

I also made the wrong assumption that URLs for the assets always look like /{path}/{name}{blah}{pos}.{_format}, so everything that is needed is in the URL. But that is plain wrong as the output path is arbitrary and need not contain anything that we can use to re-construct the asset name/position.

I don't think we can drop support for the output pattern even at debug time, as users might for example have some code that expects assets in particular (output path) places. As long as the URL for an asset is generated in Twig, one might not care about it, but there might be code (for example, an URL coded in a JavaScript) that relies on a particular path and where no other URL can be substituted.

Even if the mapping from the output path back to asset name and position was moved from the router to another request event handler as @kriswallsmith suggested above it would basically still be a Routing thing. And unless we scan all the assets to find the right one by the output path we'd probably be constructing a mapping or lookup index that we'd need to cache what brings us back to the original problem.

Shall I open a separate issue for not having to rebuild the router on asset changes or is the potential performance improvement not worth it?

What do you think about the changes to the ConfigCache/Resource implementation suggested above?

@mpdude

Issues #17 and #80 are duplicates of this one?

@mpdude

If symfony/symfony#7781 gets merged, we can

  • have a ConfigCacheFactory that passes the DIC into created ConfigCache instances
  • When the ConfigCache unserializes resources that implement ContainerAware, re-inject the DIC into them
  • Make AsseticResource ContainerAware and implement \Serializable to drop the "old" container upon serialization.
@fabpot fabpot added a commit to symfony/symfony that referenced this issue Apr 8, 2015
@fabpot fabpot feature #14178 [Config] Delegate creation of ConfigCache instances to…
… a factory. (mpdude)

This PR was squashed before being merged into the 2.7 branch (closes #14178).

Discussion
----------

[Config] Delegate creation of ConfigCache instances to a factory.

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes (refactoring, new flex point)
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | we'll see :-)
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | symfony/symfony-docs#5136

In the Routing/Router and Translation/Translator, delegate creation of ConfigCache instances to a factory. The factory can be setter-injected but will default to a BC implementation.

The ```ConfigCacheFactoryInterface``` is designed in a way that captures the common ```$cache = new ...; if (!$cache->isFresh()) { ... do sth }``` pattern. But more importantly, this design allows factory implementations to take additional measures to avoid race conditions before actually filling the cache.

By using an exchangeable ConfigCache factory it becomes possible to implement different resource (freshness) checking strategies, especially service-based ones.

The goal is to be able to validate Translators and Routers generated by database-based loaders. It might also help with symfony/assetic-bundle#168. This PR only contains the minimum changes needed, so the rest could be implemented in a bundle outside the core (at least for the beginning).

Component/HttpKernel/Kernel::initializeContainer still uses the ConfigCache implementation directly as there is no sensible way of getting/injecting a factory service (chicken-egg).

This is a pick off #7230. It replaces #7781 which was against the master branch. Also see #7781 for additional comments/explanations.

## Todo

* [ ] Allow `symfony/config` `~3.0.0` in `composer.json` for the HttpKernel and Translator component as well as TwigBundle once this PR has been merged into the master branch (fail deps=high tests for the time being).

Commits
-------

6fbe9b1 [Config] Delegate creation of ConfigCache instances to a factory.
b01ed89
@fabpot fabpot closed this Jul 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment