shutdown of Executors provided by ThreadPoolManager

[maggu2810]

While writing https://github.com/openhab/openhab-core/pull/744#discussion_r279149985 I wonder if such "mistakes" should be prevented by the framework itself.

An executor is taken from the ThreadPoolManager and e.g. used at different places. The user perhaps doesn't know that it is a shared executor and it must not be shut down.

Here my RFC:

• The class WrappedScheduledExecutorService is used by the ThreadPoolManager internally and it is the only one that uses that class, so it can be modified without affect other ones (it is also part of an internal package).
• Let's override the methods that shutdown an ExecutorService (e.g. shutdown and shutdownNow). The implementation should log a warning (or error) with a message "this method must not be called, please fix your code" and a stack trace. The specific method of the super class is not called.
• We provide a replacement for the specific methods with a prefix e.g. "really" or something similar (reallyShutdown, ...), that calls the respective super method.
• The "really shutdown" methods are only known by the ThreadPoolManager.
• The thread pool manager could provide methods like shutdownScheduler(final String poolName) that takes the pool from the pools map and calls the "real shutdown" method.
• This way it is explicit and very informative that a (shared) scheduler provided by the ThreadPoolManager should be shut down.

WDYT (e.g. all @openhab/core-maintainers and and of course all the others)?

[davidgraeff]

I'm strongly for this change. I have so many times seen this in addon reviews. I would actually kill the bundle that tries to shutdown the shared executor pool to make it very obvious to the binding developer.

[maggu2810]

I would actually kill the bundle that tries to shutdown the shared executor pool.

Do you think about something like that:

final Bundle bundle = FrameworkUtil.getBundle(getCallerClass());
bundle.stop();

I am not sure how we can get the caller class or another way to get the bundle that calls the shutdown method.

It seems Java 9 will make it possible: https://alidg.me/blog/2017/9/8/stack-walking-api-java9

We could try to load the class by name that is returned by StackTraceElement::getClassName() but I don't know if the whole approach is working.

So, can you please provide an example how you would like to kill that bundle? IMHO for the first shot a log message can be used and if someone can provide a more clever logic, we could change that.

[cweitkamp]

I agree. That is something the framework should take care about. I can remember a situation where it was possible to shutdown openHAB via System.exit(). Not very cool.

We have other places where we have to take care about similar "mistakes". E.g. the shared instances of HttpClient in HttpClientFactory or WebSocketClient in WebSocketFactory. They can be started and stopped from the outside world too.

One more thing I thought about while reading #757. I do not think that this issue is the right place to discuss but it is somehow related to the OP. What happens to scheduled jobs of Profiles? Do they remain active after a Channel will be unlinked from an Item? How is the lifecycle of a Profile? I could not find something like a deactivate method. Do we have to change that?

[davidgraeff]

final Class callerClass = Class.forName(Thread.currentThread().getStackTrace()[2].getClassName())
final Bundle bundle = FrameworkUtil.getBundle(callerClass);

Something along those lines. Stackoverflow also suggests the SecurityManager which offers a method to get the calling class.

[cweitkamp]

Regarding the HttpClient I will come up with a proposal for a new "WrappedHttpClient". Beside the stop() it might make sense to handle calls of the setters too. As the JavaDoc states they should not be use either:

https://github.com/openhab/openhab-core/blob/c547414003c43079e0c5b089b36d2e2544401b25/bundles/org.openhab.core.io.net/src/main/java/org/eclipse/smarthome/io/net/http/HttpClientFactory.java#L59-L65

[davidgraeff]

Hm. We are using a bunch of different http clients all over openhab2-addons. I recently needed to do some http requests as well in the hue emulation service.

And to be honest I did not use the jetty http client.

• It does not do anything better or easier than other client libraries. Actually the API is confusing and is not really hiding the jetty service model behind it. Naturally you are calling stop() because it sounds like dispose() or close() in other libraries to free resources.
• It does not support an async operation mode (a CompletableFuture based API)
• A wrapper would make things worse.

With Java11 we have the new java promoted http client: https://openjdk.java.net/groups/net/httpclient/intro.html

• Available on every java 11 installation.

Google promotes okhttp.

• Financially supported by Google

OSGi has it's own http client API (independent of specific implementations).

• Advantage: Can be configured for our needs like every other OSGi bundle service.

Personally I'd say we actually ban jetty. Also because of the name ;) I'm always confusing netty, jetty and jersey.

[kaikreuzer]

Personally I'd say we actually ban jetty.

We just introduced it a year ago and it was a huge effort to get rid off most of the rest. Imho we have enough regressions and instabilities, we don't need to add yet another one for no good reason.

Regarding the HttpClient I will come up with a proposal for a new "WrappedHttpClient".

@cweitkamp We tried to do all of that a while ago already - there were multiple problems, why we didn't succeed (e.g. afaik due to the Jetty class being final) and decided to have it covered by code reviews only. You should find a detailed discussion somewhere at ESH (don't have a link right now...)

[maggu2810]

OSGi has it's own http client API (independent of specific implementations).

Can you point me to the specific chapter?

[davidgraeff]

Can you point me to the specific chapter?

My bad. Not OSGi, but Jax-RS has defined their own http client. With OSGi specifying the jax-rs whiteboard in compendium R7 they have acknowledged all jax-rs API though, I guess.

[maggu2810]

With OSGi specifying the jax-rs whiteboard in compendium R7 they have acknowledged all jax-rs API though

Hm, I don't know if they acknowledged all the API or not. But for me a whiteboard integration of an API does not necessary mean that you "like" all of that API.

The JAX-RS Client API provides a high-level API for accessing any REST resources, not just JAX-RS services.

Isn't a client API for accessing REST resources just a special use case for a full future HTTP client that can be used for all non-REST HTTP use cases, too?

[davidgraeff]

Isn't a client API for accessing REST resources just a special use case for a full future HTTP client that can be used for all non-REST HTTP use cases, too?

Sure. It's a full featured http client. I guess the authors needed to put "accessing REST resources" into the description to justify that they just created the 100th java http client API.

We just introduced it a year ago and it was a huge effort to get rid off most of the rest.

Understood. There are just still so many different http client APIs in use in addons2. And it's not getting better.

The Telegram-2-binding author asked me in the community forum if he can stay with the apache http client library as it is more stable for him and his users, after he had ported the library over to jetty as well. And I agreed. Stability is more important IMO.

In the Hue emulation service I have used the jax-rs client API. Jetty for some reason didn't work in my unit tests. And to be honest I want a http client that provides a modern async API like the new Java 11 one.

I guess we did the entire http client API harmonize project to keep in control of thread spawning. How about allowing all http client APIs, but request from authors to use an openHAB controlled thread pool. Most, if not all, libraries allow to bootstrap with a given threadpool. Which brings me back to this Issue here:

We should not only prevent shutdown to be called but also unload addon bundles that spawn threads that are not coming from openHAB controlled thread pools.

[cweitkamp]

Regarding the HttpClient I will come up with a proposal for a new "WrappedHttpClient".

@cweitkamp We tried to do all of that a while ago already - there were multiple problems, why we didn't succeed (e.g. afaik due to the Jetty class being final) and decided to have it covered by code reviews only. You should find a detailed discussion somewhere at ESH (don't have a link right now...)

Alright. Then there is no need for me to spend time on it again.