HttpUtil leaks resources/threads on bundle update

[sjsf]

Since #1864 the HttpUtil class is internally using the Jetty HTTP client. However, it happily creates and starts it in https://github.com/eclipse/smarthome/blob/447b2d6e90f4714d4dee8e127173b5209292508f/bundles/io/org.eclipse.smarthome.io.net/src/main/java/org/eclipse/smarthome/io/net/http/HttpUtil.java#L340-L348

but it never stops it again. This is not much of a problem unless the o.e.sh.io.net bundle gets updated or restarted at runtime. Then the previous http client remains alive, including all its resources, and its abandoned threads keep running.

[DaHoC]

There is no quick & easy & clean solution to this. The two main issues:

1. Memory leak due to missing cleanup due to missing lifecycle (all static methods)
2. One static http client is shared among requests, whereas calls may set proxy settings -> settings of requests may interfere with each other causing requests to fail

Possible solutions:

1. Use OSGi lifecycle: Migrate this HttpUtil class into an OSGi component to allow injection and use of of the HttpClientFactory, and add a Bundle Activator lifecycle with shutdown/cleanup phase on stop() (does not solve problem 2, we would have to add some synchronization for that)
2. Use plain Java HttpURLConnection: Remove the HttpUtil static jetty http client and use simple HttpURLConnection instead
3. Use threadPool with timeout: Pass the jetty http client a threadPool whose threads have a max. lifespan (using e.g. setIdleTimeout(), setStopTimeout()) to remove stale connections (does not solve problem 2)
4. Similar to the first solution, but use the HttpClientFactory lifecycle to shutdown the client again instead of the bundle activator (does not solve problem 2)

[sjsf]

The main purpose of the HttpUtil is using it in rules etc. for having a quick, one-shot way of doing an HTTP call. That's why they methods are all static and therefore 1.) is not an option.

To me, 3.) and 4.) both looks possible in theory, but actually are quirks and only working around the underlaying problem.

I'm actually in favor of option 2.). Using a plain can also be a source for errors and leaks (that's why I'm normally an opponent of using it in bindings), but as it it fully encapsulated here I think we should be able to manage that properly in this case.

[kaikreuzer]

For users that want to go the OSGi service way, we have the HttpClientFactory available (which is a bit painful for bindings as it has to be passed through, but ok). For the rules use case, this is clearly not an option as @sjka correctly mentions.

Actually, I so far thought that the HttpUtil is already using our shared http client - the fact that it creates and starts its own is imho the main issue here. So if I get your options right, I would say that (4) should nicely address this - simply make those static methods refer to the shared instance (and throw an IllegalStateException if this service is not available). Problem 2 is imho inherent to our shared client as well, but rather unrealistic, because proxy settings should usually depend on the network of the installation and thus should be the same for all the clients.

[DaHoC]

The HttpUtils currently do not use the shared http client from the HttpClientFactory. Doing so would require the HttpUtils to be made an OSGi component to allow injecting HttpClientFactory as reference - as you said this would be solution 4. Furthermore, no setters must be called on the shared client (such as setting the proxy) - but then again you made a point in that it makes sense that if all those requests use the same network, the proxy settings should remain the same across all requests. I already implemented solutions 1, 2, and 4 in separate bugfix branches for comparison, I'll open a PR for solution 4

[sjsf]

@kaikreuzer what from your perspective is the benefit of using the shared HTTP client here?

Considering that the static methods are there for a fact, this really doesn't fit well the rest of the lifecycle story. Looking up the service via FrameworkUtil, hoping that is available at that point in time is more like a quirk than a real solution. And throwing IllegalStateExceptions won't help any user. I already see bug reports coming in because of that. And you'll be pointing to #1896 again. And all that, because... why?

[kaikreuzer]

Simple: Why should we have two shared clients in the system, each permanently consuming 5-8 threads and doing exactly the same thing?

[sjsf]

Does HttpURLConnection spawn new threads? That's new to me, I wasn't aware of that. Still, HttpURLConnection is part of the JRE, so it doesn't add any footprint.

[kaikreuzer]

I am talking about the status quo. The Jetty client spawns a lot of threads. And we had decided to use the Jetty client for HTTP requests throughout the system. I wouldn't want to reverse this (heavily discussed) decision that easily again. After all, we ask people to use HttpUtil for "simple calls" and move to the HttpClientFactory for more complex things. As we have learned in the past, changing one client implementation against another can result in many subtle and weird effects.

[sjsf]

I see. I am talking about a solution to this issue. And I don't want to reverse the decision of moving from apache http client to jetty http client. The HttpClientFactory is fine and nobody suggested changing that one.

All I'm suggesting is changing an internal implementation detail of HttpUtil. And that, to the smallest footprint http client we have at hand. This is nothing that should leak out to the caller. And as we are dealing with rather simplistic single-shot calls here only, the whole keep-alive stuff shouldn't really matter here - which in fact was what got us into trouble with jetty in combination with some flawed IoT device firmware.

[kaikreuzer]

This is nothing that should leak out to the caller.

This is what I doubt. Afair, we also had issues when switching from Apache to Jetty client in HttpUtil as there is simply a change in behavior - you cannot avoid this leaking out. Likewise, people switching from HttpUtil to HttpClientFactory will have to deal with such potential subtleties continuously, so it is not just a one time effort to fix all things that come up after changing the internal implementation.