Circular dependencies?

[mthmulders]

I'm trying to see if I can get the MVC specification (JSR 371) and its reference implementation Krazo working in a Quarkus app.
I think the right way to do that is by creating a Quarkus extension.
I've got a skeleton of that, and I have a small MVC-application (in the same repo) on top of Quarkus to see if it actually works.

When I package my app, the quarkus-maven-plugin fails with:

[error]: Build step io.quarkus.arc.deployment.ArcAnnotationProcessor#build threw an exception: java.lang.IllegalStateException: Circular dependencies not supported: 
 PRODUCER FIELD bean [types=[java.lang.Object, org.eclipse.krazo.uri.ApplicationUris], qualifiers=[@Default, @Any], target=org.eclipse.krazo.uri.ApplicationUris org.eclipse.krazo.uri.UriTemplateParser.applicationUris, declaringBean=org.eclipse.krazo.uri.UriTemplateParser] injected into: org.eclipse.krazo.MvcContextImpl
 CLASS bean [types=[org.eclipse.krazo.uri.UriTemplateParser, java.lang.Object], qualifiers=[@Default, @Any], target=org.eclipse.krazo.uri.UriTemplateParser] injected into: org.eclipse.krazo.uri.UriTemplateParser
 CLASS bean [types=[javax.mvc.MvcContext, java.lang.Object, org.eclipse.krazo.MvcContextImpl], qualifiers=[@Named(value = "mvc"), @Default, @Any], target=org.eclipse.krazo.MvcContextImpl] injected into: org.eclipse.krazo.lifecycle.RequestLifecycle, org.eclipse.krazo.uri.UriTemplateParser, org.eclipse.krazo.cdi.RedirectScopeManager, org.eclipse.krazo.binding.validate.ValidationInterceptor
 CLASS bean [types=[org.eclipse.krazo.lifecycle.RequestLifecycle, java.lang.Object], qualifiers=[@Default, @Any], target=org.eclipse.krazo.lifecycle.RequestLifecycle] injected into: org.eclipse.krazo.cdi.AroundControllerInterceptor

I've been poking around a little bit to understand what's going on.

• Quarkus tries to instantiate the MvcContextImpl bean.
• That class has an instance field applicationUris.
• The UriTemplateParser class produces a value for that; its @PostConstruct-annotated init method is responsible for that.
• But this class uses the MvcContext to do that.

If you follow Krazo's Install Guide for Servlet Containers, you'll get the Weld implementation for CDI.
The same sample app works just fine in that environment.
From what I've seen during debugging, Weld builds a proxy for every injection that you define.
It only resolves that bean when there's an actual interaction with it, say a method invocation.

A few questions here:

1. Would an Quarkus extension indeed be the way to go if I want to achieve server-side rendering of pages with JSR-371?
2. Is the circular bean message in this scenario that Quarkus could/would be able to handle?
3. If so, what would be right place to start digging in Quarkus?

[manovotn]

Not really an answer to your question, but just chiming in...
What I know for a fact is that Quarkus (or more accurately Arc) doesn't support circular dependencies at the moment whereas Weld does so long as there is at least one normal scoped bean in a given dependency chain (which is strictly speaking what CDI specification requires).

I am not sure if there are plans to support that (@mkouba might know more?) as it is a corner case (not frequently used) and in many cases designing with circular dependencies is simply a fishy design in the first place.

[mkouba]

Matej is right, circular dependencies are not supported ATM. He's also right that users are encouraged to avoid circular dependencies where possible. On the other hand, it's a CDI spec requirement and so we should probably think about how difficult it would be to add the support of circular dependencies.

[mthmulders]

To be clear, I totally agree with @manovotn that having circular dependencies is not something to wish for. Sure, there will be exceptions to the rule anyway.

In this case I've brought op the issue because it's an external codebase (Krazo) which I obviously cannot easily change. Of course I could mention it there, but they're probably well aware of the fact and I'm not sure if they're willing to change it.

[mkouba]

I fully understand ;-). And I also doubt that Krazo's team is going to try to get rid of circular deps because as I mentioned - every CDI implementation must support it.

[manovotn]

After some discussion with @mkouba - the current implementation operates with InjectableBean as a metadata reference. Each such bean knows all of its dependencies and holds them in a form of InjectableReferenceProvider fields. In order to support cyclic dependencies, we would need to replace this with something like Supplier<InjectableReferenceProvider>. With that we would also need to change the validation to detect these and verify them accordingly, but that's the easy part.

It's definitely a feature that we would like to look into sometime in the future but for now it is of lower priority.
That being said, contributions are of course very welcome :-)

[mthmulders]

Thanks for all the pointers! I'm willing to give it a try and see if I can implement something like this. I just can't make any promises on the timeframe...

[mthmulders]

I've started working on this on a branch in my fork of Quarkus. I've refactored the CurrentInjectionPointProvider to hold a Supplier<InjectableReferenceProvider> and passes all tests (except for the one that I added).

Now I'm wondering what the dependency graph should look like. The ComponentsProviderGenerator builds a Map<BeanInfo, List<BeanInfo>> beanToInjections and a Map<BeanInfo, ResultHandle> beanToResultHandle, but the loop that builds them breaks in case of a circular dependency. How to avoid that? Simply removing the isDependency check results in quite a few tests breaking because Gizmo tries to allocate local vars for handles without a type. Maybe that's the expected situation but I'm probably overseeing something here...

[manovotn]

If you are talking about validation inside generate() method, then you need to temper the detection of actual circular dependency by changing when boolean stuck gets set to true.

From CDI specification, an unsupported circular dependency is one where all beans are of pseudo-scope. If there is at least one normal scoped bean in the chain, then it should be supported. See this spec bit.

So you will need to gather information on scopes. There can be custom scoped added too. For each bean you inspect you can get its ScopeInfo telling you what scope it has (and whether it is normal scope) and from there you should be able to determine if the circular chain has at least one normal scoped bean in it.

[mthmulders]

Hmm, but where should we get the ResultHandle from in the addBean method? The beanToResultHandle map doesn't yet contain the bean that is being added so we can't generate appropriate bytecode for it.

(By the way, if this discussion is better held on Zulip, please say so and we'll take it there)

[manovotn]

Hmm, my understanding was that you do it the same way except that instead of keeping track of injection points for each bean via InjectableReferenceProvider, you swap to Supplier<InjectableReferenceProvider> instead. Is that right @mkouba?
E.g. this is where Arc generates the IPs and where it should instead create suppliers.

[mthmulders]

E.g. this is where Arc generates the IPs and where it should instead create suppliers.

I've changed that place to work with Supplier instead of InjectableReferenceProvider. After some additional changes, unit tests are still green.

But my question remains. The addBean method expects that its beanToResultHandle param will contain a ResultHandle for its bean parameter. But in case of a circular dependency, it isn't there (yet). What should the generated getComponents method of the generated ComponentProvider do in that case?

[manovotn]

Sorry for the delay, the notification slipped my attention.

You're going to have to replace the Map<BeanInfo, ResultHandle> beanToResultHandle with something like Map<BeanInfo, Supplier<ResultHandle>> beanToResultHandleSupplier.
And then we'll only call the supplier once we really need it which would be after the generation.

EDIT: hold on, I am wrong. The ResultHandle itself will be the supplier. You would need to create the supplier in a bytecode basically, then store it in the map. Ideally that would only be done for the beans with circular deps (if possible) because having lambdas is an overhead.

[mthmulders]

Yeah, figured out that you must've meant something like the edit instead of the original comment :-).

Having lambdas is indeed quite some overhead but I don't really know if we should have a Collection of ResultHandle instances where some of them refer to a Supplier instance and some of them to a *_Bean instance. Wouldn't that complicate things?

Anyway, I think I have the circular dependencies working. I've replaced the List<InjectableBean> in *_Components#getComponents with a Map<String, Supplier<InjectableBean<?>>>> - so we can do a runtime lookup of the required bean. The key in this map is the bean identifier.

Would you like me to create a pull request so we can discuss the implementation and see whether/where it needs improvement?

[manovotn]

Yeah, figured out that you must've meant something like the edit instead of the original comment :-).

Having lambdas is indeed quite some overhead but I don't really know if we should have a Collection of ResultHandle instances where some of them refer to a Supplier instance and some of them to a *_Bean instance. Wouldn't that complicate things?

That's what we ideally want so that with no circ deps you have no extra lambdas in place.
So in the end it should simplify things maybe :)

Anyway, I think I have the circular dependencies working. I've replaced the List<InjectableBean> in *_Components#getComponents with a Map<String, Supplier<InjectableBean<?>>>> - so we can do a runtime lookup of the required bean. The key in this map is the bean identifier.

Would you like me to create a pull request so we can discuss the implementation and see whether/where it needs improvement?

Sure! Bring up a draft PR and let's go from there.

[mthmulders]

I've created the draft PR.

I'll see if I can get rid of the many lambda's being created currently. Maybe things can be simpler than I thought at first - we'll see. If so, I'll update the PR.

[mthmulders]

I noticed that there had been quite some activity on the master branch since I started my work. I'm rebasing against master first and will create a new PR afterwards.