Introduce remote thread pool and option to deactivate pool limits

[kaikreuzer]

Replaces #129

Also-by: morph166955 rosenblumb@gmail.com
Signed-off-by: Kai Kreuzer kai@openhab.org

[kaikreuzer]

@morph166955 I've just tested this on my prod system and it seems to work nicely - system shutdown is not blocked anymore and I do not notice any issues.

May I ask you to review my PR to make sure that I didn't miss anything essential from your prior work?

[morph166955]

@kai, no glaring issues. I like the use of Executors.newCachedThreadPool(). If I'm reading the code right, you create a new and unbound thread pool that can grow without an upper limit (which is basically the same as I was doing by creating new threads however it takes advantage of a cached pool which is even better!). I don't have a system up to test this on right now so I can't confirm that it all works as expected, but it looks like it should! Either way, I like it!

We need to make a few more additions to add the remote executor in "all the right places". If I'm tracking correctly from #129 it has to be changed in bundles/org.jupnp/src/main/java/org/jupnp/registry/RemoteItems.java on lines 111, 167, 217, and 231.

The only thing I'm seeing otherwise is that it looks like there's a bunch of lines in here that are formatting changes caused by the editor that aren't relevant to the commit itself (e.g. line 120-128 looks like an indention change). We may want to revert those out to make the commit cleaner.

Awesome job and thank you for all the work!

EDIT: Don't change getRegistryListenerExecutor() like I said originally, that will cause other problems. Just replace it in RemoteItems and I "think" that should be ok.

EDIT 2: Small note. PR should be named "Introduced remote thread pool and option to deactivate pool limits".

[morph166955]

I attempted to push the change from here but don't have permissions. I was able to do it easily with

sed -i 's/getRegistryListenerExecutor/getRemoteListenerExecutor/g' bundles/org.jupnp/src/main/java/org/jupnp/registry/RemoteItems.java

hope that makes it easy/fast.

[kaikreuzer]

Thanks @morph166955, I have done the changes.
Will deploy it to my prod env and keep it running for a few days before merging (and doing a new jUPnP release).

[morph166955]

Awesome! Thank you!

[kaikreuzer]

Would be great it you could test as well. Atm I am having issues with the SamsungTV binding, this does not seem to work anymore. Not sure if it is due to jUPnP, will have to investigate...

[morph166955]

I'll compile a jar in a bit and load it. I got rid of my Samsung TVs when i moved so I won't be able to help there. I'm also now running in a docker container on a Synology so let's see how all this is going to work.

[morph166955]

I've compiled against 6638d9e and updated my OH3 to 2408 to make sure I'm on the newest snapshot. My Sonos speakers are being weird (two are offline) but they also show as offline in the Sonos app so I don't think it's related. I'll report back if I can't clear it up.

[morph166955]

I fixed the devices being offline. My speaker was the culprit.

I've noticed one issue, but I can't say that it's related to jupnp or not. I added a few extra docker containers to my synology today and when they started to load up my sonos speakers flapped very quickly (less than a second). When I left the system quiet, everything was stable so I'm blaming this on the system not having enough juice. I'll keep on testing with this jar, but other than this hiccup which I think is unrelated, everything looks clean and stable.

[kaikreuzer]

I have some good news from my SamsungTV as well: For some reason, I had to re-pair it (grant permission to openHAB). Now it all seems to be working.

[morph166955]

That's awesome! I'll keep chugging here. Trying to learn how to do a stack dump on my synology so I can see how all of this looks.

So is the plan to commit this, do the 2.6.0 release, and then do the PR to add this to OH 3.1 before the GA drop?

[kaikreuzer]

Yes, that's the plan. I'll wait for your verdict and if you think it all looks good on your machine, I'll merge and do the release the next days.

[morph166955]

My logs have been completely clear since last night. No instability at all so I don't think we have any regressions. My event log shows normal speaker events. I have yet to find an easy way to do a jstack dump with OH running inside the docker container on my synology. The OH docker package doesn't seem to have jstack in the path. So minus doing a stack dump to see that the threads are all being created properly, I believe we're in good shape.

[kaikreuzer]

Ok, thanks for your input - I'd then say let's dare to merge as all looks good so far!

[morph166955]

Agreed. Fingers crossed. Next step is the leap to get this into the OH3 snapshot and see how it does on a wider scale.

[kaikreuzer]

Release is done! https://github.com/jupnp/jupnp/releases/tag/2.6.0

[morph166955]

Wahoo!