YAML things provider: create things even if binding is not yet installed #4753
Conversation
...e.model.yaml/src/main/java/org/openhab/core/model/yaml/internal/YamlModelRepositoryImpl.java
Outdated
Show resolved
Hide resolved
...e.model.yaml/src/main/java/org/openhab/core/model/yaml/internal/YamlModelRepositoryImpl.java
Outdated
Show resolved
Hide resolved
|
I put this PR in draft mode because I encounter this strange exception: while I don't have it in main branch! |
|
The problem is because things are updated while some of them have their handler already initializing. I should also study if there is a way to update only models that are really impacted. If what triggers the need of a refresh is the start of a new binding, ideally only models containing things for this binding should be refreshed. |
|
I've been meaning to ask you about this. As a result of this, if you added a Thing without a loaded Binding, this Thing will not appear in the system at all. You cannot see it in the UI either of course. Only when you've installed the binding will you then register the Thing and see it in the system (Thing Registry). This behaviour is not the same as Managed Things. Managed Things will register the Thing regardless of whether the Binding / Thing Handler exists, so that you can see it in the ThingRegistry and also on the UI. This gives users a chance to know that they are missing the binding and the UI provides a handy link to install the binding. IMO this is a nicer behaviour than "hiding" unknown Things like it is currently done in YAML / DSL. It will also probably solve your circular reference issue and significantly simplify the code. |
Not at all unfortunately because a refresh will be needed in any case when the new thing handler is started, something that can happen at any time after the server is started (as soon as you install a new binding). I would say that it is another point I have myself mentioned earlier but with no direct link with the current problem. |
|
I have an idea how to avoid reloading all YAML files containing things when a binding is added. The idea is to reload only the things attached to this binding, whatever the YAML file it belongs to. |
|
I don't know if this is a desirable approach here, I'd just like to add that in general, it might not be beneficial to make complex solutions to avoid circular references. The alternative is to not make the reference optional, using ReferenceCardinality, ReferencePolicyOption and ReferencePolicy as needed. The only caveat is that that means that the implementation must handle that the reference might be |
I will think to that. But the problem is more than the circular reference, it is what models to refresh when a new binding is loaded. I converged to a solution that is not bad I believe, more simple than what I described in my previous message. |
As I said, it was just a general suggestion for such situations, if and when it is a better solution. I didn't study the case here so that I could understand what is and isn't a good approach 😉 |
|
I think you're right, I will restore the reference but will make it optional. I will also add code to finally refresh only what is needed. |
lolodomo
force-pushed
the
fix_circular_ref
branch
2 times, most recently
from
f9877d8 to
cec746d
Compare
|
Ok, I made the reference optional and enhanced the system to refresh only needed things. |
|
@Nadahar : do you know if injection of references can occur in parallel (in different threads) or if they are done in sequence ? |
I don't know. This is normally only done during startup/shutdown, or if somebody stops/starts/restarts a bundle while the system is running. I haven't seen any thread safety measures when I've looked at other code that deals with (de)registration in OH, but that doesn't mean that it's safe. The information about the topic seems to be somewhat sparse (which usually means it's unsafe), but I can find several indications that there is no protection here: My assumption has been that it is indeed unsafe, that is, that these methods are called by various threads, but that since it's only during startup and shutdown this happens, exposure is "limited" so you usually "get away with it". I do remember having had quite a "random factor" getting OH to start in the past, especially back in OH2, and I've considered it likely that this has been threading issues all along (since those are the ones that usually are quite unpredictable in their ways). Not all classes gets their references set in the constructor, so even in cases where references are mandatory, there should be a small gap where the object has been created but the reference is invalid. Maybe OSGi has some kind of protection against this case, in that the object "isn't made available" until madatory references has been set for example, but I don't know. Have you experienced some issues or are you just being cautious? IMO it never hurts to assume that things can be called concurrently, unless you know that it's not the case. |
|
Ok, thank you for your answer. |
Thinking about it more, it could simplify the problem and the implementation if the things were created even without a thing handler. I will go in that direction. |
lolodomo
changed the title
Makes it consistent with managed thing provider. Removes the OSGi reference between YamlThingProvider and YamlModelRepository and as consequence removes the circular reference. Signed-off-by: Laurent Garnier <[email protected]>
|
I finally changed the YAML thing provider to create the things even when the binding is not yet installed. It makes it consistent with the managed thing provider. And I removed the reference creating a circular reference. |
|
@openhab/core-maintainers : is it possible please to have a review of this PR ? |
Signed-off-by: Laurent Garnier <[email protected]>
Makes it consistent with managed thing provider.
Removes the OSGi reference between YamlThingProvider and YamlModelRepository and as consequence removes the circular reference.
Closes #4799