Skip to main content


Eclipse Community Forums
Forum Search:

Search      Help    Register    Login    Home
Home » Eclipse Projects » Eclipse SmartHome » Questions about best practices implementing bindings
Questions about best practices implementing bindings [message #1775340] Fri, 27 October 2017 20:48 Go to next message
Hilbrand Bouwkamp is currently offline Hilbrand BouwkampFriend
Messages: 4
Registered: October 2017
Junior Member
I posted these questions on the openHAB forum. At the advice of @kai I post this question here on the smart home forum.
---
I've been building some bindings and review code recently and looked at other reviews and have some questions about best practices:

Threads

It is in general not allowed to create your own threads (Point 2 of https://www.eclipse.org/smarthome/documentation/development/guidelines.html#d-runtime-behavior). The point has an exemption clause to start a discussion on the forum. For binding developers and reviewers it would great if this is made more specific. Because I don't see a lot of discussions but see people creating their own threads. Meaning threads are simply created anyway. Therefore I also want to start the discussion and see if the usage can be made more specific. Here are 2 recent cases I encountered:

In the Rotel PR there is a thread needed that just loops forever, blocks until input from the serial bus is received, handles it and blocks again. The suggested solution was not to use schedule or threadpool but: Executors.newSingleThreadScheduledExecutor(). It this ok? and why? and how should it be implemented to not have dangling threads?
In the Blebox PR in discovery the local network is scanned. If done sequential this would take 8 minutes, therefore the implementation does this concurrently with 50 threads so it only takes 10 seconds. Here also the Executors.newFixedThreadPool(THREADS_NUMER) is used. Is this the right approach? and if not how should it be implemented?

Catching Exception

Very regular I see bindings implement catch(Exception e). This is considered bad. In general these catches are done in background threads (for example to poll the state of a device) to make sure the thread doesn't get killed. One way is to only catch checked exceptions, but that leaves out the RuntimeExceptions. What is the preferred solution: Only catch checked exceptions and let the RuntimeExceptions stop the thread or also catch RuntimeExceptions?

Logging to error

According to the coding style logging to error should only be done when something is wrong with the setup. My interpretation of this is that a binding should simply never log to error, as a single binding failing isn't great but not a severe bug. Is this correct?

Checking configuration parameters

If a binding configuration parameter has a default and is required the value will never be null when the initialize of the binding is called because otherwise a configuration validator will already have thrown an exception. Is it therefore necessary to tests these values on null values? Or can these checks be omitted? (There are other check to think of, like min) I would say they can be omitted making the code simpler.
Re: Questions about best practices implementing bindings [message #1775429 is a reply to message #1775340] Mon, 30 October 2017 08:31 Go to previous messageGo to next message
Kai Kreuzer is currently offline Kai KreuzerFriend
Messages: 648
Registered: December 2011
Senior Member
Quote:
The suggested solution was not to use schedule or threadpool but: Executors.newSingleThreadScheduledExecutor(). It this ok?


For anything that continuously listens on some incoming data (like e.g. a tcp socket or a serial port connection), it is imho a typical exception to the rule, i.e. using a dedicated thread is absolutely fine (and cannot be avoided). No need to go for a SingleThreadScheduledExecutor either as no discrete jobs are scheduled.

Quote:
the implementation does this concurrently with 50 threads [...] and if not how should it be implemented?


Scanning the whole network should normally not be done at all. You have no clue how big the network is and what your flooding might cause (it might even consider openHAB to be an attacker and block it).
Devices on the network should provide some proper means to announce themselves, e.g. by UPnP or mDNS. If they do not offer this, they should be expected not to be auto-discoverable, so that the binding should simply ask the user for the IP address of the device.

Quote:
What is the preferred solution: Only catch checked exceptions and let the RuntimeExceptions stop the thread or also catch RuntimeExceptions?


This very much depends on your code at hand. You should always try to handle specific exceptions. So if there are RuntimeExceptions that might be thrown from your own logic and from which you can recover, those can be caught. For anything else, like OOMs etc. it probably is better to let them bubble up and thus terminate the thread.

Quote:
According to the coding style logging to error should only be done when something is wrong with the setup.


If a binding expects some information e.g. from the file system or from the database and it receives only invalid data or e.g. lacks permissions or whatever, it is perfectly fine to log an error.
E.4 also specifically mentions the case of bugs that the user should report. So any path in your code that you do not expect that should ever be executed should do some error logging, so that it can be reported.

Quote:
Or can these checks be omitted?


Afaik, no, because the config description must be considered optional and not necessarily to be present/checked. The code should always make sure that the config it receives is valid and what it expects. The framework does not give any assertions.
Re: Questions about best practices implementing bindings [message #1775491 is a reply to message #1775429] Mon, 30 October 2017 22:24 Go to previous messageGo to next message
Hilbrand Bouwkamp is currently offline Hilbrand BouwkampFriend
Messages: 4
Registered: October 2017
Junior Member
Kai Kreuzer wrote on Mon, 30 October 2017 08:31
No need to go for a SingleThreadScheduledExecutor either as no discrete jobs are scheduled.

Actually you suggested to use this yourself ;-): https://github.com/openhab/openhab2-addons/pull/1241#discussion_r137763231 (Or I misrepresented the use case here with what you intended).

So my guess would be to use something like newSingleThreadExecutor in such a case. Then In a handler start it in the initialize method and stop/cancel it in dispose method?

In another case, when a device is temporarily not reachable a thread sleep is used to wait for some time and try to connect again. I would say this could also be done with the newSingleThreadScheduledExecutor and start the blocking process with scheduleWithFixedDelay, and when the device is not reachable simply return from the run thread and use the schedule delay to restart the process after that delay. This would omit the need to sleep (and handling checked Interrupted Exception that sleep throws).

Kai Kreuzer wrote on Mon, 30 October 2017 08:31
Scanning the whole network should normally not be done at all. You have no clue how big the network is and what your flooding might cause (it might even consider openHAB to be an attacker and block it). Devices on the network should provide some proper means to announce themselves, e.g. by UPnP or mDNS. If they do not offer this, they should be expected not to be auto-discoverable, so that the binding should simply ask the user for the IP address of the device.


Beside the one in the pr I found several openhab2 bindings that do seem to scan the network. Shouldn't that be removed then?: dscalarm, zway, russound and silvercrestwifisocket

Kai Kreuzer wrote on Mon, 30 October 2017 08:31
So if there are RuntimeExceptions that might be thrown from your own logic and from which you can recover, those can be caught. For anything else, like OOMs etc. it probably is better to let them bubble up and thus terminate the thread.

In scheduled threads and such if an exception is thrown and not catched within that thread the thread will simply stop, but the error will not be logged. This can lead to hard to find bugs. A solution can be to or always catch RuntimeException in the thread in the binding code or let the framework provide a scheduler that does log these exceptions on those unchecked exceptions (or even update the state of a binding. Although the latter might be not always the right state) What would be a good solution?

Kai Kreuzer wrote on Mon, 30 October 2017 08:31
If a binding expects some information e.g. from the file system or from the database and it receives only invalid data or e.g. lacks permissions or whatever, it is perfectly fine to log an error.
E.4 also specifically mentions the case of bugs that the user should report. So any path in your code that you do not expect that should ever be executed should do some error logging, so that it can be reported.

This makes sense. The wording in E.4 on error logging seems somewhat stronger than this, but that might be my interpretation. From that perspective I would like to restate it somewhat differently. What do you think?:

error logging should be used to inform the user that something is wrong, and the system cannot function normally anymore, and there is a need for immediate action. e.g. from the file system or from the database and it receives only invalid data or e.g. lacks permissions or whatever. It means the code fails irrecoverably and the user fix the problem if possible (e.g. disk was full) or else if it can't be solved by the user report it as a bug.

Kai Kreuzer wrote on Mon, 30 October 2017 08:31
Afaik, no, because the config description must be considered optional and not necessarily to be present/checked. The code should always make sure that the config it receives is valid and what it expects. The framework does not give any assertions.


Do you mean by optional that the developer didn't provide it or that it's not available at runtime? Because in case the developer didn't provide it I understand it. But then if it's provided by the developer can we assume the framework will guarantee the restrictions set in the config. E.g. required means when initialize method is called it will not be null. It might be the value is still not correct, but at least the null check can be omitted? Or do you mean it's not checked (at runtime), even when the developer did provide a configuration? That doesn't sound like I would expect it to work though (You mentioned checked, which i could interpret that you do mean it's not available at runtime even when provided by the developer) .
Re: Questions about best practices implementing bindings [message #1775932 is a reply to message #1775491] Wed, 08 November 2017 11:29 Go to previous message
Kai Kreuzer is currently offline Kai KreuzerFriend
Messages: 648
Registered: December 2011
Senior Member
Quote:
Actually you suggested to use this yourself ;-): https://github.com/openhab/openhab2-addons/pull/1241#discussion_r137763231 (Or I misrepresented the use case here with what you intended).


Almost. I actually suggested newSingleThreadExecutor, not newSingleThreadScheduledExecutor there. Using newSingleThreadExecutor was the easiest way to refactor your existing code. Implementing a thread on your own is usually more difficut and error-prone. So I chose the "safe" option as an advice there. But what I said above is still correct: There is no need to use newSingleThreadExecutor and ideally one should spawn ones own dedicated perpetually running thread.

Quote:
Then In a handler start it in the initialize method and stop/cancel it in dispose method?


Yes, that should be it.

Quote:
In another case, when a device is temporarily not reachable a thread sleep is used to wait for some time and try to connect again. I would say this could also be done with the newSingleThreadScheduledExecutor and start the blocking process with scheduleWithFixedDelay


This is one of the main use cases why we provide the "scheduler" in the BaseThingHandler. In such a situation, a handler indeed must not block a thread (neither a self-created one nor one from the thread pool), but work with scheduled jobs instead.

Quote:
Beside the one in the pr I found several openhab2 bindings that do seem to scan the network. Shouldn't that be removed then?


Well, people implemented it there and it seemed to work for them and provides value. If I am asked, I do not suggest doing it that way for the given reasons. I am reluctant to forbid it though and to force them to remove that code. What I ask people though is to make sure that it is not done as an automatic background discovery, but that it only becomes active if a user deliberately triggers a scan for devices of that certain binding. This reduces the risk that the whole system is impacted by this scan although the user does not actually use it.

Quote:
Although the latter might be not always the right state) What would be a good solution?


I don't have an educated answer here. If the main issue is that exceptions that cause a thread to terminate are not logged, it might be an option to enhance our QueueingThreadPoolExecutor to actually do such a logging. It's better to address it in the framework than having to do it in every single implementation of a job.

Quote:
From that perspective I would like to restate it somewhat differently. What do you think?:


Fine for me, feel free to create a PR against the guideline page.

Quote:
Do you mean by optional that the developer didn't provide it or that it's not available at runtime?


I mean potentially not available at runtime. The config descriptions are really meant as meta-data to enable UIs to deal with the system. Their purpose is NOT to be a contract for the runtime side. So "max" actually means that a UI should mandatorily limit a selection range to this maximum value. But if people use other means (e.g. textual through files) of configuring their system, there is nothing that prevents them setting a higher value for this config parameter in their definition. So the runtime code should be able to deal with that.

There is some exception, tough: The "required" parameters are considered by the framework itself and handlers are only initialized, if all required parameters are provided. This is done in order to avoid "early starts" of handlers, while a configuration might still be in the progress of being incrementally filled (e.g. through some step-by-step wizard). But any further validity checks are the job of the handler.
Previous Topic:How to read / save Binding config
Next Topic:Identical Things provided by different ThingProviders
Goto Forum:
  


Current Time: Tue Jun 19 05:10:33 GMT 2018

Powered by FUDForum. Page generated in 0.01478 seconds
.:: Contact :: Home ::.

Powered by: FUDforum 3.0.2.
Copyright ©2001-2010 FUDforum Bulletin Board Software

Back to the top