Skip to content

[DependencyInjection][VarExporter] Generate lazy-loading virtual proxies for non-ghostable lazy services #47236

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 3, 2022
Merged

[DependencyInjection][VarExporter] Generate lazy-loading virtual proxies for non-ghostable lazy services #47236

merged 1 commit into from
Sep 3, 2022

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Aug 9, 2022 •
edited
Loading

Q A
Branch? 6.2
Bug fix? no
New feature? yes
Deprecations? no
Tickets -
License MIT
Doc PR -

Since #46752 and #46751, we are able to make services lazy out of the box, except when 1. a service relies on an internal class 2. a service has the proxy tag or 3. a service's class is abstract (and the service uses a factory). In these situations, proxy-manager-bridge was required. This was an acceptable trade-off because this would be quite uncommon. But while working on Doctrine, I realized that we cannot use ghost objects when a factory is used. I described this for Doctrine in doctrine/orm#9896 but the situation can happen with any services constructed by a factory. This means we'd need proxy-manager-bridge anytime a factory is used on a lazy service. What was uncommon becomes quite common and the trade-off is not acceptable anymore. Thus this PR.

This PR adds a LazyProxyTrait and a ProxyHelper to build lazy loading virtual proxies at will. It then wires this new capability into the container. As a result proxy-manager-bridge is not needed anymore. We can deprecate it in another PR.

While the diff is quite big, LazyProxyTrait has many similarities with LazyGhostTrait and both traits can be diffed to see where their behavior varies.

Excerpt from the README for the record:

The component provides two lazy loading patterns: ghost objects and virtual proxies (see https://martinfowler.com/eaaCatalog/lazyLoad.html for reference.)

Ghost objects work only on concrete and non-internal classes. In the generic case, they are not compatible with using factories in their initializer.

Virtual proxies work on concrete, abstract or internal classes. They provide an API that looks like the actual objects and forward calls to them. They can cause identity problems because proxies might not be seen as equivalents to the actual objects.

Because of this identity problem, ghost objects should be preferred when possible. Exceptions thrown by the ProxyHelper class can help decide when it can be used or not.

Ghost objects and virtual proxies both provide implementations for the LazyObjectInterface which allows resetting them to their initial state or to forcibly initialize them when needed. Note that resetting a ghost object skips its read-only properties. You should use a virtual proxy to reset read-only properties.

@carsonbot carsonbot added this to the 6.2 milestone Aug 9, 2022
@nicolas-grekas nicolas-grekas force-pushed the ve-inheritance-proxy branch 3 times, most recently from 603bf18 to c863918 Compare August 9, 2022 21:43
@nicolas-grekas nicolas-grekas mentioned this pull request Aug 10, 2022
Merged
@nicolas-grekas nicolas-grekas mentioned this pull request Aug 10, 2022
Closed
@nicolas-grekas nicolas-grekas force-pushed the ve-inheritance-proxy branch 15 times, most recently from ce2a8a2 to 95a92da Compare August 11, 2022 12:31
@alexander-schranz
Copy link
Contributor

This looks very interesting. While we are using the Messenger middlewares a lot and so a lot of services are initialized as all middlewares are always loaded in your our sync bus. I was thinking about making arguments lazy.

I currently workaround it by using getSubscribedServices as that seems only initialize the service when being used but not so a fan of it: https://github.com/sulu/messenger/pull/3/files#diff-f24c4e9e2f4ac12bb6ca919142c7689d57ebe7eb12024b46cc8bacbd9ff03e9a

So I thought if it would be possible to set something lazy only for a specific service:

<service id="sulu_messenger.lock_middleware" class="LockMiddleware">
      <argument type="service" id="lock.factory" lazy="true" />
</service>

Or what would be your recommendation in this kind of case?

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Aug 11, 2022 •
edited
Loading

a lot of services are initialized as all middlewares are always loaded in your our sync bus.

IIRC, we read the middleware stack iteratively, aka we instantiate individual middleware layers on demand. Is this broken?

I currently workaround it by using getSubscribedServices

That's The use case for this interface so it's perfectly fine.

<argument type="service" id="lock.factory" lazy="true" />

Yes, that's the kind of innovations that having this in core would allow!
This would require the argument to be typed and we'd inject a virtual proxy instead of the real instance (unless the target service is already lazy). PR welcome as a follow up ;)

@nicolas-grekas nicolas-grekas force-pushed the ve-inheritance-proxy branch 3 times, most recently from 6a4743f to 961b521 Compare August 30, 2022 16:10
@nicolas-grekas nicolas-grekas force-pushed the ve-inheritance-proxy branch 3 times, most recently from 7980f1b to 53cd184 Compare August 31, 2022 06:29
@nicolas-grekas nicolas-grekas changed the title [DependencyInjection][VarExporter] Generate lazy proxies for non-ghostable lazy services out of the box [DependencyInjection][VarExporter] Generate lazy-loading virtual proxies for non-ghostable lazy services out of the box Aug 31, 2022
@nicolas-grekas nicolas-grekas changed the title [DependencyInjection][VarExporter] Generate lazy-loading virtual proxies for non-ghostable lazy services out of the box [DependencyInjection][VarExporter] Generate lazy-loading virtual proxies for non-ghostable lazy services Aug 31, 2022
@nicolas-grekas
Copy link
Member Author

For the record I had a look to split lazy-traits from var-exporter and honestly there's too much code is common. Splitting would double the maintenance effort and bugs would need to be solved twice. Making one depend on the other would require opening APIs that are currently internal. I tried I promise, but technically, this is not sound to me.

@nicolas-grekas nicolas-grekas force-pushed the ve-inheritance-proxy branch 8 times, most recently from 20e1df3 to 3d29386 Compare September 2, 2022 14:25
@fabpot
Copy link
Member

fabpot commented Sep 3, 2022

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 1198986 into symfony:6.2 Sep 3, 2022
@fabpot fabpot mentioned this pull request Sep 3, 2022
Closed
@nicolas-grekas nicolas-grekas deleted the ve-inheritance-proxy branch September 3, 2022 10:24
nicolas-grekas added a commit that referenced this pull request Sep 12, 2022
@nicolas-grekas
minor #47292 [Cache] make Redis*Proxy extend Redis* (nicolas-grekas)
ee78099 
This PR was merged into the 6.2 branch.

Discussion
----------

[Cache] make `Redis*Proxy` extend `Redis*`

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #47267, #42428
| License       | MIT
| Doc PR        | -

`Redis*Proxy` did not extend native classes because we missed the code infrastructure to generate appropriate proxies.
With #47236, we now have it. This PR adds generated proxies to the cache component to keep it lazy-able out of the box.

Commits
-------

cfb46ed [Cache] make `Redis*Proxy` extend `Redis*`
@fabpot fabpot mentioned this pull request Oct 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@chalasr chalasr chalasr approved these changes

@yceruto yceruto Awaiting requested review from yceruto yceruto is a code owner

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

Assignees
No one assigned
Labels
DependencyInjection Feature Status: Reviewed VarExporter
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.
7 participants
@nicolas-grekas @alexander-schranz @fabpot @stof @soyuka @chalasr @carsonbot