Don't use scheduler to execute command threads

[cdjackson]

This uses a thread to execute the command rather than the scheduler which is not suitable for real-time commands.
Closes #392
Signed-off-by: Chris Jackson chris@cd-jackson.com

[hsudbrock]

It would be really interesting to understand why these huge delays that you mention in #392 occur (it still sounds somewhat implausible to me, but I also don't have a good argument why the delays should not be caused by the scheduler provided by the BaseThingHandler).

I think the change in this PR is OK, but would it be OK for you to change to using a local scheduler, as you suggested in the discussion in #392? (I also think that using a scheduler might be the better solution, to avoid the overhead of thread creation on systems where many commands need to be handled by the ZigBee binding).

[cdjackson]

It would be really interesting to understand why these huge delays that you mention in #392

I agree, but I've no way to find this out. I can only assume that with the 19 bindings that are running, something is taking a lot of time, or scheduling a lot of tasks, or... However, as the system is not mine, I have no way to find out.

What I can say is that the quick fix to use the thread solved the problem and commands were implemented quickly.

I will update to use a local scheduler.

[hsudbrock]

What I can say is that the quick fix to use the thread solved the problem and commands were implemented quickly.

That sounds good :)

[cdjackson]

I've updated this PR but haven't had the chance to test it - will do so in the weekend if necessary.

[hsudbrock]

With this change, a thread pool will be created for each zigbee thing handler instance. Shouldn't we rather use a shared pool for all zigbee thing handler instances, as follows (where the ThreadPoolManager is from package org.eclipse.smarthome.core.common), wdyt?

private ExecutorService commandScheduler = ThreadPoolManager.getPool("zigbee-thinghandler-commands");

As a side note, a colleague of mine mentioned that the thread pools returned by newCachedThreadPool turned out to be problematic in an OSGi setup, because they caused class loader leaks at least in our setting (but I don't know the details about those issues).

[cdjackson]

I guess it’s ok. I was trying to avoid any dependence between things. I’ll update if you think it is advantageous

…

Sent from my iPhone

On 19 Feb 2019, at 03:09, Henning Sudbrock ***@***.***> wrote: @hsudbrock commented on this pull request. In org.openhab.binding.zigbee/src/main/java/org/openhab/binding/zigbee/handler/ZigBeeThingHandler.java: > @@ -129,6 +131,8 @@ private boolean firmwareUpdateInProgress = false; + private ExecutorService commandScheduler = Executors.newCachedThreadPool(); With this change, a thread pool will be created for each zigbee thing handler instance. Shouldn't we rather use a shared pool for all zigbee thing handler instances, as follows (where the ThreadPoolManager is from package org.eclipse.smarthome.core.common), wdyt? private ExecutorService commandScheduler = ThreadPoolManager.getPool("zigbee-thinghandler-commands"); As a side note, a colleague of mine mentioned that the thread pools returned by newCachedThreadPool turned out to be problematic in an OSGi setup, because they caused class loader leaks at least in our setting (but I don't know the details about those issues). — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[hsudbrock]

I see your point to avoid dependencies between things (in particular, as it is probably dependencies between bindings via the BaseThingHandler scheduler that caused the issue leading to this PR).

I still think we should use a shared pool at least for all ZIgBee things, because I fear problems due to too many threads on slow hardware with many paired ZigBee devices if we don't share the pool.

[cdjackson]

But this doesn’t reduce threads does it? Won’t the scheduler just allocate as many threads as are requested - I didn’t think there was a limit? The advantage of the cached scheduler is only that threads are reused (at least that’s what I thought?). If we wanted to limit threads then we need a different scheduler I think (although I can’t check the code at the moment as I’m on a plane!).

…

Sent from my iPhone

On 19 Feb 2019, at 13:20, Henning Sudbrock ***@***.***> wrote: I see your point to avoid dependencies between things (in particular, as it is probably dependencies between bindings via the BaseThingHandler scheduler that caused the issue leading to this PR). I still think we should use a shared pool at least for all ZIgBee things, because I fear problems due to too many threads on slow hardware with many paired ZigBee devices if we don't share the pool. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[hsudbrock]

The advantage of the cached scheduler is only that threads are reused (at least that’s what I thought?)

I think so as well. But that means that if there are currently no threads used by a thing handler, I think that the pool for that handler will not immediately remove the unused threads from the pool, they will still be in the pool to be reused when needed. (Some pools will reduce the number of threads after some timeout.) That is, if each thing handler has its own pool as currently implemented, a system with, say, 50 ZigBee devices will have 50 pools with a total of 50*(average number of idle hreads per pool) threads on the system, which can be a lot for not-so-powerful systems. In contrast, when using one pool for all ZigBee thing handlers, there will be less idle threads in that single pool.

Or do I make a thinking error here?

[cdjackson]

I'm not sure I'm convinced that it's a big issue, but on the other hand, you're right that it will use less threads. I'll change it, and if we find there are performance issues, we can always update again (although if there are, further investigation would probably be in order ;) ).

[hsudbrock]

lgtm, thanks Chris!

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/hue-binding-for-api-v2-3-4-0-0-4-0-0-0/146799/15

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/hue-binding-for-api-v2-3-4-0-0-4-0-0-0/146799/30