Circular reference detected #870
Comments
|
Definitely something to look at. |
|
The same issue here. [ERROR] [org.eclipse.smarthome.core.transform] - [org.eclipse.smarthome.action.transformation.action(72)] Circular reference detected, getService returning null |
|
I just caught a full exception that could help us in the analysis: The culprit might be the ScriptActivator as it appears in the middle of the stacktrace. |
|
This is what appears to be happening: The bundle org.eclipse.smarthome.core.transform is being started. This bundle provides a service which implements org.eclipse.smarthome.model.script.engine.action.ActionService. That interface class is located in the bundle org.eclipse.smarthome.model.script, which has been configured to start when one of its classes are loaded, so the loading of this interface class triggers the start of the org.eclipse.smarthome.model.script bundle. The issue also seems to occur in other places: Some potential ways to work around this...
Personally, I'm not a big fan of service trackers in an activator, so option 3 would have my preference. |
|
@maggu2810 Already introduced DS-controlled activators for the model.runtime bundles, so in general option 3 make sense (as long as the services are immediately activated). We might need to add some null checks to e.g. the static methods in BusEvents etc. @maggu2810 Was there any reason why you left the Activator in place for the model.script bundle or do you think it is safe to change as well? |
|
Besides the activators, the issue also occurs in the ServiceBinder class (see my stacktrace above). I'm not sure I understand the purpose of this class. Why not use standard DS and keep everything consistent or move to DS annotations rather than reinventing the wheel? |
|
Question: If there are circular dependencies between services, DS cannot help us either, can it? |
|
It will depend on the cardinality/policy used. If there is a static policy on both ends of a circular dependency, then in my opinion some refactoring is due... |
|
Can you figure out which case we have here before spending to much effort on refactoring towards DS? |
|
For the issue with the org.eclipse.smarthome.action.transformation.action, there is no circular dependencies between services, but between a bundle and a service. In this case, using a dynamic 0..n policy in the org.eclipse.smarthome.model.script should work just fine. |
|
You mean specifically for the dependency to ActionService, yes. The other three are 1..1 dependencies though. |
Yes, so those should be static.
I will try to work on this tomorrow.
OK. |
|
I'm a bit stuck getting this to work. After the refactoring, the ScriptEngineOSGiTest is failing on a script parse error. It looks to me that the error message is not correct, but as I don't know Groovy or xtend, I'm hitting a brick wall here... |
|
Just to clarify: the code changes in the pull request do work when used in openHAB. It's only the test case which is failing. |
|
Groovy and Xtend stuff in tests are indeed ugly to analyze... I'll have a try to find out what is going on there. Thanks for the PR! |
Have to look at the code, but after a very short look at #947 we have to check that the order is still strict. |
|
I just had a look at the classes that use these trackers - a few are instantiated by Guice, which is now started up by the script.runtime component that @maggu2810 has created. So maybe we should introduce a dependency of the runtime.xml to the new service factory component - this way, we should be sure that the startup order is as required. |
|
OK, I can add that. I don't think that will fix the test case though. I tried the same test case in plain Java to make sure the sequence was correct: It results in the same script parsing error in xtend as above. Not sure where to look next. |
|
What you are missing in your plain Java test case is the definition of the item "Switch1" - this might be the same problem for the failing test above. |
|
Thanks for the pointer. It's building without test errors now. I still need to add the runtime.xml dependency. |
That's good news!
If you manage to do it before tonight, we would be able to still get it into the OH2 beta2, which I want to build tonight. |
|
Tonight won't be feasible, I gave it a go, but adding the runtime.xml dependency is not working and I have a plane to catch. I'm experiencing some weird sequencing issue in the build process. For example I get the following compilation error: After this, I don't make any changes, but just restart the build again from the failure point: and now, it magically compiles and continues the build until it reaches the ScriptEngineOSGiTest which has test failures. There is very little useful information shown, it looks like something (guice?) is swallowing exceptions, which makes this hard to troubleshoot. |
|
@dvanherbergen You shouldn't use Maven as long as the Eclipse IDE is running (and automatically build is enabled). |
|
@maggu2810, thanks for the tip. I will give that a go... |
|
ThingSetupManager is the main one. It is used by others like SetupConsoleCommandExtension, which looks like it should be deprecated. |
|
Yes. I am in the process of having it deprecated, but we are not there yet - the Paper UI also has many dependencies on it. |
Through the TypeResolver, indeed. This is not nice and should indeed be refactored. I have created #1012 for this. |
|
@dvanherbergen & @maggu2810 I lost this issue a bit out of sight, but this is still an annoying startup problem of OH2. |
|
#947 indeed only solves the first occurrence of the circular reference. By removing that issue, it revealed the other one in the ServiceBinder. As an alternative to #947, you could disable the 'activate plugin when classes loaded' as mentioned in this comment. |
|
I think we should drop "activate plugin when classes loaded" and move to DS (at the moment for all involved bundles). |
|
The initial reported circular reference could be solved by: |
|
That's what I had hoped as well, but I am still seeing the very same exception in the latest OH2 distro :-( |
|
@kaikreuzer Could you test the linked distribution at #1407 |
|
As mentioned in #1393 (comment), this looks good! |
|
Finally fixed by #1407 - many thanks, @maggu2810! |
|
Would like to add this link, I think it is very an interesting (and related) discussion: https://bugs.eclipse.org/bugs/show_bug.cgi?id=490062#c12 |
|
Interesting discussion indeed. But their statement "it means that BAPL has no impact in plain OSGi when all bundles are automatically activated" somehow does not seem to be true if I look at our issue here? |
|
The guys are much more familiar about the OSGi framework, but after reading the "Technical Solution" here (https://www.osgi.org/developer/design/lazy-start/), I can't believe that it has no impact at all. |
|
So shall we challenge them? |
|
@kaikreuzer FYI: Using equinox install the most ESH features results in a circular reference detected message. If I install the same features using felix, I don't get this error. |
|
Are you silently suggesting me to switch OH to Felix...? |
|
No, this has not been my intention. I just want to inform you about my observations. I think it would be a good start to use DS instead of the Activator (#947), so we can have a good dependency chain. |
|
I can confirm the problem, and in addition to that, the following (partly copied) is thrown as well: |
|
Thanks @kgoderis. The message you given is the result of the circular reference. One part of the circular reference chain cannot get the service. It will be disappear as soon as the circular reference is resolved. |
I don't know if it is important or not, but when I start openHAB2 beta 1, the first message I get in the logs is:
13:06:14.339 [ERROR] [org.eclipse.smarthome.core.transform] - [org.eclipse.smarthome.action.transformation.action(73)] Circular reference detected, getService returning null
The text was updated successfully, but these errors were encountered: