Home » Archived » Eclipse SmartHome » Questions about best practices implementing bindings
|
| Re: Questions about best practices implementing bindings [message #1775429 is a reply to message #1775340] |
Mon, 30 October 2017 08:31   |
 Kai Kreuzer
Messages: 673
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 |
Hilbrand Bouwkamp
Messages: 5
Registered: October 2017 |
Junior Member |
|
|
Kai Kreuzer wrote on Mon, 30 October 2017 08:31No 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:31Scanning 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:31So 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:31If 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:31Afaik, 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 |
Kai Kreuzer
Messages: 673
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.
|
|
|
Goto Forum:
Current Time: Thu Dec 12 04:54:53 GMT 2024
Powered by FUDForum. Page generated in 0.03815 seconds |
] 
Search
Help
Register
Login
Home
