Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Refactoring bridge/thing life cycle. #2087

Merged
merged 3 commits into from Nov 11, 2016
Merged

Refactoring bridge/thing life cycle. #2087

merged 3 commits into from Nov 11, 2016

Conversation

sbussweiler
Copy link
Contributor

  • A BridgeHandler of a bridge is initialized before ThingHandlers of its child things are initialized.
  • A BridgeHandler is disposed after all ThingHandlers of its child things are disposed.
  • A bridge handler (new interface BridgeHandler) is notified if a child handler is initialized/disposed.
  • The methods ThingHandler.bridgeHandlerInitialized() and ThingHandler.bridgeHandlerDisposed() are removed. This breaks the API.
  • OSGi service registration of thing/bidge handlers has been removed.

Signed-off-by: Stefan Bussweiler s.bussweiler@external.telekom.de

@maggu2810
Copy link
Contributor

maggu2810 commented Sep 5, 2016

Hi @sbussweiler,
I see a lot of

Received update for non-existing item: Item '...' could not be found in the item registry

now.
After that messages, I see

Enabling channel item provider.

and all the

Item '...' has been added.

That is a little bit confusing.

I need to inspect if the updates are generated by my handlers or the framework itself.

If them are generated by my handlers:
My thing handlers call updateState(channelIdWithoutGroup, state); so if the framework tries to update items, it already knows the item names that are linked to the channels and tries to delegate that updates but the items does not yet exist.
If the item is created I would assume that a refresh is called, so the thing handler is able to send the status update of the channel linked to that item again (we added this to the framework some time ago, see #1677). But if I look at the item state it is NULL.

@kaikreuzer
Copy link
Contributor

@maggu2810 Upon some analysis, I would assume that these log messages are due to #2096 (and not due to this PR). In #2096, I introduced a small delay before the ChannelItemProvider creates its items. The ThingManager assumes that if there is a link, the referenced item MUST exist as well and thus it sends events for it.
I am not sure how to solve this best; we currently cannot technically guarantee that items are always loaded/created before any link is loaded/created. The solution might also be related to the discussion in #1896, i.e. when we can be sure that all stuff is fully loaded after startup.

@maggu2810
Copy link
Contributor

@kaikreuzer I created #2121 to continue that topic as it seems to be not related (as you already stated) to this PR.

@maggu2810
Copy link
Contributor

@sbussweiler The normal startup works fine.
One question about the state transmissions:

  • if the bridge goes offline all my thing handlers' status is changed to: OFFLINE (BRIDGE_OFFLINE)
  • if the bridge goes online again, all my thing handlers' status is changed to: OFFLINE

If my bridge goes online again, I would like to reinitialize my thing handlers (so call dispose(), initialize() -- is this okay?).

What is the correct check?
Using bridgeStatusChanged and check for ONLINE?
I assume bridgeStatusChanged is only called if a thing has been already initialized.

@maggu2810
Copy link
Contributor

@sbussweiler Realized your comment in the other thread a little bit to late

ThingHandler.bridgeStatusChanged() is never called before a child thing has been initialized. The first bridge status change to ONLINE/OFFLINE is initiated by the bridge initialization itself. In that case ThingHandler.bridgeStatusChanged() is not called (because it is not really a change), but ThingHandler.initialize() of all child things. I refactored this behavior this morning, please pull the latest changes.

@sbussweiler
Copy link
Contributor Author

@maggu2810 thanks for your feedback!

  • if the bridge goes offline all my thing handlers' status is changed to: OFFLINE (BRIDGE_OFFLINE)
  • if the bridge goes online again, all my thing handlers' status is changed to: OFFLINE

This is the default behavior implemented in BaseThingHandler.bridgeStatusChanged() which can be overridden by any handler implementation.

What is the correct check?
Using bridgeStatusChanged and check for ONLINE

Correct!

If my bridge goes online again, I would like to reinitialize my thing handlers (so call dispose(), initialize() -- is this okay?).

In general initialize and dispose should only be called by the framework (ThingManager). But as you mentioned, it could make sense to execute some logic of initialize and dispose if your bridge goes ONLINE again. In that case extract the logic into some private methods and call them in bridgeStatusChanged(ONLINE) and in initialize() / dispose().

@maggu2810
Copy link
Contributor

@sbussweiler thanks for your feedback, too.
Are there any special scenarios you would like to get tested by me?
I will do some tests this morning myself but I do not see (till now) any blocker.

@kaikreuzer Do we need any further checks before we can merge this?

@sbussweiler
Copy link
Contributor Author

sbussweiler commented Sep 7, 2016

Are there any special scenarios you would like to get tested by me?

Most of the testing scenarios are:

  • Add things first, then bridge, check that all child thing handlers are initialized.
  • Delete bridge and check that all child thing handlers are disposed.
  • Restart binding and check correct disposal & initialize order of bridge and child things.
  • Check that bridge handler is notified about child thing initialization and disposal.
  • Play around with Xtext models which leads to an immediate bridge/thing initialization.

@kaikreuzer
Copy link
Contributor

@kaikreuzer Do we need any further checks before we can merge this?

As it is a huge change in the inner core, I'd also like to spend some time to review and test it before it is merged - I didn't yet find the time to do this, though.

@maggu2810
Copy link
Contributor

@sbussweiler Can you merge your branch of this PR and this one https://github.com/maggu2810/smarthome/tree/binding-test?
My binding-test branch contains a simple(st) binding to play with things etc.

The test-thing-bridged thing type requires a bridge:
https://github.com/maggu2810/smarthome/blob/63584e607e969d42b765477071d0763194927831/extensions/binding/org.eclipse.smarthome.binding.test/ESH-INF/thing/thing-types.xml#L16

  • I started with a clean environment (no DSL items / things / ..., no mapdb in the userdata, ...).
  • I added a thing of type test:test-thing-bridged
  • I don't select a bridge (there is no one)
  • The thing handler's initialize method is called.
14:20:20.194 DEBUG o.e.s.c.t.i.ThingRegistryImpl[:245] - Creating thing for type 'test:test-thing-bridged'.
14:20:20.262 DEBUG o.e.s.c.t.i.ThingManager[:427] - Thing 'test:test-thing-bridged:827ad070' is tracked by ThingManager.
14:20:20.262 DEBUG o.e.s.c.t.i.ThingManager[:570] - Calling 'TestHandlerFactory.registerHandler()' for thing 'test:test-thing-bridged:827ad070'.
14:20:20.264 DEBUG o.e.s.c.c.ThreadPoolManager[:145] - Created thread pool 'safeCall' with size 5
14:20:20.266 INFO  s.event.ThingAddedEvent[:43] - Thing 'test:test-thing-bridged:827ad070' has been added.
14:20:20.269 DEBUG o.e.s.c.c.ThreadPoolManager[:115] - Created scheduled thread pool 'thingHandler' of size 3
14:20:20.273 DEBUG o.e.s.c.t.i.ThingManager[:668] - Finished loading meta-data of bundle 'org.eclipse.smarthome.binding.test'
14:20:20.277 INFO  s.event.ThingStatusInfoEvent[:43] - 'test:test-thing-bridged:827ad070' updated: INITIALIZING
14:20:20.279 INFO  s.e.ThingStatusInfoChangedEvent[:43] - 'test:test-thing-bridged:827ad070' changed from UNINITIALIZED to INITIALIZING
14:20:20.280 DEBUG o.e.s.c.c.r.AbstractManagedProvider[:62] - Added new element test:test-thing-bridged:827ad070 to ManagedThingProvider.
14:20:20.283 DEBUG o.e.s.c.t.i.ThingManager[:735] - Calling initialize handler for thing 'test:test-thing-bridged:827ad070' at 'org.eclipse.smarthome.binding.test.internal.handler.TestBaseThingHandler@5a1bfb1e'.
14:20:20.289 INFO  o.e.s.b.t.i.h.TestBaseThingHandler[:21] - Thing initialize: thing uid = test:test-thing-bridged:827ad070, bridge uid = null
14:20:20.293 INFO  s.event.ThingStatusInfoEvent[:43] - 'test:test-thing-bridged:827ad070' updated: ONLINE
14:20:20.293 INFO  s.e.ThingStatusInfoChangedEvent[:43] - 'test:test-thing-bridged:827ad070' changed from INITIALIZING to ONLINE

Shouldn't this PR solve such a problem that a thing with a mandatory bridge is initialized without a bridge?
Is my test case wrong?

@sbussweiler
Copy link
Contributor Author

I added a thing of type test:test-thing-bridged

How did you added the child thing? Via REST call? E.g. in PaperUI adding a child thing first, won't work AFAIK!? You have to specify the bridgeUID for the thing, otherwise the thing is not recognized as a child of a bridge. The ThingManager checks via Thing.getBridgeUID() if a thing is a child of a bridge or not.

@sbussweiler Can you merge your branch of this PR and this one https://github.com/maggu2810/smarthome/tree/binding-test?

You mean I should merge your branch into this PR branch? I didn't had time to have a look on it, sorry. Your branch is providing some test infrastructure for thing/bridge life cycle?

@maggu2810
Copy link
Contributor

You mean I should merge your branch into this PR branch?

No, not to this PR as it is not related.
If you want to give it a try yourself, you can merge this branch with a test binding to your working copy.

How did you added the child thing? Via REST call? E.g. in PaperUI adding a child thing first, won't work AFAIK!?

I was able to add this thing using the Paper UI and leave the bridge selection box empty.
As a referenced bridge could be removed and the thing remains, I don't know if the framework must prevent creation of "bridged-things" without a bridge.
But at least the initialization method should not be called if a bridge is not specified or a specified is missing.

@kaikreuzer
Copy link
Contributor

I don't know if the framework must prevent creation of "bridged-things" without a bridge.

I think this is what would need to happen. If there is no bridge referenced in the Thing, there is no chance to tell whether a bridge is required or not - so the framework correctly assumes the Thing can do without a bridge and thus instantiates the handler for it.
If a bridge is mandatory, the bridgeUID must imho always be filled out.

Copy link
Contributor

@maggu2810 maggu2810 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only minor comments, LGTM

* through a {@link Bridge}.
* A {@link Thing} is a representation of a connected part (e.g. physical device or cloud service) from the real world.
* It contains a list of {@link Channel}s, which can be bound to {@link Item}s.
* </p>
Copy link
Contributor

@maggu2810 maggu2810 Sep 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC: <p> should be used only (there is no closing paragraph tag in JavaDoc

"If you have more than one paragraph in the doc comment, separate the paragraphs with a <p> paragraph tag, as shown."
Quote: http://www.oracle.com/technetwork/articles/java/index-137868.html

The closing paragraph tag occurs multiple times in this PR, I don't comment the other occurrences.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

* Creates a {@link ThingHandler} for the given thing.
*
* @param thing
* thing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we don't need a line break between parameter name and its description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes..

* @param thing
* thing
* @param thing the thing that should be handled, not null
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we drop that empty line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The empty lines are not required, but from my point of view it makes the javadoc/code more readable.

* @param callback
* {@link ThingHandlerCallback} which must be used for the {@link ThingHandler}
* @param thing the thing for which a new handler must be registered
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we drop that empty line?

* @param thing the thing for which a new handler must be registered
*
* @return the created thing handler instance, not null
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we drop that empty line?

* @param configuration configuration
* @param thingUID thing uid, which can be null
* @param bridgeUID bridge uid, which can be null
*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we drop that empty line?

logger.error(
"Exception occured while disposing handler of thing '" + thing.getUID() + "': " + ex.getMessage(),
ex);
logger.error("Exception occured while disposing handler of thing '" + thingHandler.getThing().getUID()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been wrong before your commit, but as you touched this line could you use {} instead of string concatenation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, will change commented error logs.

logger.error("Exception occured during status update of thing '" + bridgeChildThing.getUID()
+ "': " + ex.getMessage(), ex);
logger.error("Exception occured during notification about bridge status change on thing '"
+ child.getUID() + "': " + ex.getMessage(), ex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been wrong before your commit, but as you touched this line could you use {} instead of string concatenation?

} catch (Exception ex) {
logger.error("Exception occured during bridge handler notification ('" + bridge.getUID()
+ "') about handler initialization of child '{}' '" + thing.getUID() + "': "
+ ex.getMessage(), ex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been wrong before your commit, but as you touched this line could you use {} instead of string concatenation?

} catch (Exception ex) {
logger.error("Exception occured during bridge handler notification ('" + bridge.getUID()
+ "') about handler disposal of child '{}' '" + thing.getUID() + "': "
+ ex.getMessage(), ex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has been wrong before your commit, but as you touched this line could you use {} instead of string concatenation?

@sbussweiler
Copy link
Contributor Author

@maggu2810 thanks for your comments, I'm back from vacation and will have a look on it!

*
* @param componentContext
* component context (must not be null)
*/
protected void deactivate(ComponentContext componentContext) {
for (ServiceRegistration<ThingHandler> serviceRegistration : this.thingHandlers.values()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, isn't dropping the thingHander service registrations a problem here?
If the factory goes down (because some required services are not available anymore), this meant in the past that handlers were also unregistered and hence their Things go into UNINITIALIZED/HANDLER_MISSING status.

Now, it seems that the handlers would be kept, so a binding has no possibility to dispose them anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no problem. If a factory goes all handlers are unregistered by the ThingManager. See method ThingManager.removeThingHandlerFactory().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the implicit assumption that a ThingHandlerFactory does not have any dynamic dependencies, but that it is deactivated on any change of its dependent services. I think this must be clearly mentioned in the documentation - it is probably best if you could add a new section that explains how to deal with service dependencies in handlers (i.e. that they do not have a bundleContext anymore and thus should not themselves deal with their dependencies, but only get them reached in from the factory through their constructor).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes the implicit assumption that a ThingHandlerFactory does not have any dynamic dependencies, but that it is deactivated on any change of its dependent services.

Correct - if service references are handed over to the handlers (what will be the case in general). Services only used by the factory could be dynamic.

I think this must be clearly mentioned in the documentation

Yes, I need to make it obvious.

ThingHandler thingHandler = createHandler(thing);
if (thingHandler == null) {
throw new IllegalStateException(this.getClass().getSimpleName()
+ " could not create a handler for the thing '" + thing.getUID() + "'.");
}
setHandlerContext(thingHandler);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we do not treat the handler as an OSGi service anymore, I wonder if we really still need to provide the bundleContext to it? Actually, all code places that I found that are using it look at bit fishy to me, so this rather seems to provoke bad smells...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct! I will remove the BundleContext.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the bundle context from the BaseThingHandler causes the following refactoring:

  • The BaseThingHandlerFactory must handle the tracking of the ThingRegistry and the ItemChannelLinkRegistry.
  • Due to the fact that the BaseThingHandlerFactory is an abstract class we can not use declarative service features. This would end in a crappy implementation (service tracker must be used, the factory must be unregistered if the ThingRegistry disappears, etc.)

This leads me to the following question: should a binding developer use core services like ThingRegistry, ItemChannelLinkRegistry and ItemRegistry?

My proposal would be:

  • A binding should not use ESH core services directly.
  • Required functionality of such core services could be provided by the framework.
  • E.g. the ThingHandlerCallback provides methods (currently used in the BaseThingHandler) like
    • ThingHandlerCallback.getBridge(ThingUID bridgeUID)
    • ThingHandlerCallback.isItemLinked(String channelId)
    • ... further use cases TBD
  • BundleContext, ThingRegistry, ItemChannelLinkRegistry would be removed from the BaseThingHandler.

Problem:

  • This will break a lot of bindings, because BundleContext, ThingRegistry, ItemChannelLinkRegistry are protected members in the BaseThingHandler.
  • ThingHandlerCallback.getBridge(ThingUID bridgeUID) and ThingHandlerCallback.isItemLinked(String channelId) are the obvious use cases covered by the BaseThingHandler. Further uses cases must be analyzed (which bindings are using the protected members for what?).

Due to the fact the this refactoring will break a lot of bindings, I would like to provide this refactoring in a separate PR. The changes of this PR are quite enough.

@kaikreuzer what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BundleContext, ThingRegistry, ItemChannelLinkRegistry would be removed from the BaseThingHandler.

This imho makes sense as these services should not be directly available to binding developers.

This will break a lot of bindings

I hope that it doesn't affect to many bindings after all - from a quick check in the ESH+OH2 repos I only found 4 that would need to be changed.

I would like to provide this refactoring in a separate PR.

If this is something that is pretty isolated from the other changes, doing it in a separate PR is fine for me - I'd just like to have your confirmation that you'll nonetheless continue to work on it :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed with @kaikreuzer: BundleContext, ThingRegistry and ItemChannelLinkRegistry are removed from the BaseThingHandler in a separate PR.

* Handles a command for a given channel.
* Disposes the thing handler, e.g. deallocate resources.
* </p>
* The framework expects this method to be non-blocking and return quickly. For longer running disposals,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really? For a dispose this is rather unexpected to me - I would have assumed that when the method returns, the instance can really be thrown away and that it does not do any further activities in the background.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In most of the cases dispose is a short running operation. The intention was to provide the possibility to handle also long running disposals.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would not allow long running disposals. After all, this should NOT do any communication with the device, it should only clean up internal memory etc. It should be a valid assumption that the instance can be GC'ed immediately after dispose returns.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

* of this handler does not have a bridge, this method is never called.
*
* @param thingStatusInfo the status info of the bridge
* This method is called before a thing will be removed. An implementing class can handle the removal in order to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"before a thing is removed" instead of "before a thing will be removed"

@lsafelix75
Copy link

Any guide how to get this feature to be tested on my OpenHab2 IDE environment?

@kgoderis
Copy link
Contributor

Small friendly ping as to where are standing with this as it is a requirement to solve #1847?

@kaikreuzer
Copy link
Contributor

afaik, we are still waiting for an update from @sbussweiler, is this right, Stefan?

@sbussweiler
Copy link
Contributor Author

Correct, there is an ongoing discussion about a refactoring. I will add this as soon as possible.

@sbussweiler
Copy link
Contributor Author

@kaikreuzer thanks for re-testing.

In general, code in this method is most likely not required anymore, since it all happens already in initialize() (since the handler is now always there first), correct?

Correct.

@maggu2810
Copy link
Contributor

You can also use this opportunity to again update the tests that are now shown in conflict again :-/

Didn't realize that we should not merge other stuff - but perhaps it would be the best if this one is nearly ready to merge.

@kaikreuzer
Copy link
Contributor

Didn't realize that we should not merge other stuff

No, it is fully ok to merge other stuff - we just have to resolve conflicts of PRs afterwards that are waiting for CQ approval, that's life.

Signed-off-by: Stefan Bussweiler <s.bussweiler@external.telekom.de>
Signed-off-by: Stefan Bussweiler <s.bussweiler@external.telekom.de>
@sbussweiler
Copy link
Contributor Author

Despite the fact that I asked you not to do any changes to your branch, it is actually ok to add a commit to it to adapt this line - just make sure that you keep the initial commit as is, i.e. do not squash anything. You can also use this opportunity to again update the tests that are now shown in conflict again :-/

Done. Resolved merge conflicts and changed bridgeStatusChanged() default behavior.

@kaikreuzer
Copy link
Contributor

FTR: Once the CQ is approved, we should merge this with keeping the 3 commits in place.

@kaikreuzer kaikreuzer removed the CQ label Nov 11, 2016
@kaikreuzer
Copy link
Contributor

Alright, we have check-in approval - now it counts @sbussweiler ;-)

@kaikreuzer kaikreuzer merged commit 9bf9cac into eclipse-archived:master Nov 11, 2016
@kaikreuzer kaikreuzer deleted the thing-bridge-lifecycle branch November 11, 2016 13:44
kaikreuzer added a commit to openhab/openhab-addons that referenced this pull request Nov 16, 2016
@sjsf sjsf mentioned this pull request Dec 7, 2016
Jamstah pushed a commit to Jamstah/openhab-addons that referenced this pull request Dec 12, 2016
@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants