eclipse-archived / smarthome

Eclipse SmartHome™ project
https://www.eclipse.org/smarthome/
Eclipse Public License 2.0
862 stars 784 forks source link

Replace multiple HttpClients with Jetty #1689

Closed svilenvul closed 7 years ago

svilenvul commented 8 years ago

The problem is discussed already in the forum. I could not find any opened other close issue about that. I will paste here some of the posts:

Jochen Hiller wrote on Sat, 08 August 2015 09:30

Hi all,

I am just working on getting some bindings running on a non-Equinox OSGi runtime environment. I struggled over that the Eclipse SmartHome framework and some bindings (from ESH, from openHAB2, ignoring openHAB1 for the moment) are using different HttpClients. To minimize the footprint over restricted devices (like Raspberry Pi 1) it is a bad idea to use multiple clients at same time, will increase memory footprint and startup time.

I made a rough analysis of the code base:

bundles using Apache HttpClient 3.x $ find . -name "MANIFEST.MF" -exec grep -l "org.apache.commons.httpclient" {} \; ./openhab2-master/git/openhab2/addons/binding/org.openhab.binding.autelis/META-INF/MANIFEST.MF ./openhab2-master/git/openhab2/addons/binding/org.openhab.binding.squeezebox/META-INF/MANIFEST.MF ./openhab2-master/git/openhab2/bundles/core/org.openhab.core/META-INF/MANIFEST.MF ./openhab2-master/git/openhab2/bundles/core/org.openhab.core.compat1x/META-INF/MANIFEST.MF ./smarthome-master/git/smarthome/bundles/io/org.eclipse.smarthome.io.net/META-INF/MANIFEST.MF ./smarthome-master/git/smarthome/bundles/ui/org.eclipse.smarthome.ui/META-INF/MANIFEST.MF

bundles using Apache HttpClient 4.x $ find . -name "MANIFEST.MF" -exec grep -l "org.apache.http." {} \; ./openhab2-master/git/openhab2/addons/binding/org.openhab.binding.ipp/META-INF/MANIFEST.MF

bundles using Jetty Client $ find . -name "MANIFEST.MF" -exec grep -l "org.eclipse.jetty.client" {} \; ./openhab2-master/git/openhab2/addons/binding/org.openhab.binding.avmfritz/META-INF/MANIFEST.MF

I also checked the different footprint of the used http clients:

  • Apache HttpClient 3.x: org.apache.commons.httpclient, org.apache.commons.codec (about 600 kB)
  • Apache HttpClient 4.x: org.apache.httpcomponents.httpclient, org.apache.httpcomponents.httpcore, org.apache.commons.codec (about 1,5 MB)
  • Jetty Client 9.2.x: Jetty Client, Http, IO, Util (about 900 kB)

As HttpClient 3.x is considered as legacy this should be removed long term. So decision would be between Apache HttpClient 4 and Jetty Client. Considering the footprint Jetty would be first choice, but the client libraries might be not so popular like Apache HttpClient. In general such a change would break the API in one case (HttpUtil.createHttpMethod), but this will not be used until now in ESH, openHAB2 (only 1x used in openHAB1 NEST binding), so no real issue.

What do you think? Is this the effort worth to try to get rid of not needed HttpClients? Same time new bindings should be asked/enforced to stick on same http client.

Bye, Jochen

Kai Kreuzer wrote on Sat, 08 August 2015 21:03

Hi Jochen,

I think we all agree that we should get rid off Apache HttpClient 3 - thanks for highlighting this. Regarding the question of the best replacement: So far we had documented that Apache HttpClient 4 should be used: [url]https://www.eclipse.org/smarthome/documentation/development/bindings/dependencies.html[/url] But there were reasons why some bindings used the Jetty library instead. I think I just recalled them: What you have missed is that the Apache HttpClient 4 does not support asynchronous processing of requests - you will need an additional component called asyncclient ([url]https://hc.apache.org/httpcomponents-asyncclient-4.1.x/index.html[/url]), which ends up at 2MB with all its dependencies. Furthermore, the version 4.1 somehow suggests that it is not very actively maintained (not in sync with the HttpClient 4 at least - the issue tracker shows last activity in February this year: [url]https://issues.apache.org/jira/browse/HTTPASYNC/?selectedTab=com.atlassian.jira.jira-projects-plugin:summary-panel[/url]).

The Jetty client seems to be much more active and modern - it e.g. already comes with HTTP/2 support as well and many other features, see [url]https://webtide.com/http2-support-for-httpclient/.[/url] I personally have some positive experiences with using this library (asynchronously) and thus would think that it probably would be a good choice. But I would love to hear some other opinions and experiences as well!

Cheers, Kai

karel goderis wrote on Tue, 18 August 2015 20:21

The Tesla binding uses javax.ws.rs.client (via WebTarget), which is I guess the Jersey client implementation in the end The Helios binding uses WS-Notification implementation from the Apache CXF library (that is pure SOAP, not clear what http client that is, and no clue how to switch that over) The HDAnywhere binding relies on org.eclipse.smarthome.io.net.http.HttpUtil The Miele binding is based on JSON-RPC, implemented via java.net.HttpURLConnection The ATSAdvanced binding (will not be published due to SDK license constraints) is based on the Apache CXF as well

I would like to work on the issue. As the last post is almost an year old, I will take a look one more time at the existing state, and most importantly, which bindings will be affected.

kaikreuzer commented 8 years ago

Thanks for looking into this!

svilenvul commented 8 years ago

The current state looks like this:

Bundles using Apache HttpClient 3.x (org.apache.commons.httpclient) - 6:
openhab2-addons\addons\binding\org.openhab.binding.autelis\META-INF\MANIFEST.MF openhab2-addons\addons\binding\org.openhab.binding.squeezebox\META-INF\MANIFEST.MF smarthome\bundles\io\org.eclipse.smarthome.io.net\META-INF\MANIFEST.MF smarthome\bundles\io\org.eclipse.smarthome.io.net.test\META-INF\MANIFEST.MF smarthome\bundles\ui\org.eclipse.smarthome.ui\META-INF\MANIFEST.MF smarthome\extensions\binding\org.eclipse.smarthome.binding.fsinternetradio\META-INF\MANIFEST.MF

Bundles using Apache HttpClient 4.x (org.apache.http.) - 1:
openhab2-addons\addons\binding\org.openhab.binding.ipp\META-INF\MANIFEST.MF

Bundles using Jetty Client (org.eclipse.jetty.client) - 3:
openhab2-addons\addons\binding\org.openhab.binding.avmfritz\META-INF\MANIFEST.MF openhab2-addons\addons\binding\org.openhab.binding.homematic\META-INF\MANIFEST.MF openhab2-addons\addons\io\org.openhab.io.myopenhab\META-INF\MANIFEST.MF

Bundle using javax.ws.rs.client:
openhab2-addons\addons\binding\org.openhab.binding.tesla\META-INF\MANIFEST.MF

I could not find any info about : Miele, Helios and ATSAdvanced.

As it can be seen, most of the bundles depend on the old Apache HttpClient 3.x. I will start digging into this list, but the main problem remains how we can assure, that no existing functionality is broken? Will developers would like to test the bindings after an eventual change?

kaikreuzer commented 8 years ago

Please ONLY regard the bundles in smarthome and openhab2-addons repository - for openHAB 1, it is absolutely ok to use something else and openhab-core simply requires this for the compatibility layer.

svilenvul commented 8 years ago

OK, I was not sure if I can ignore them, I will edit the list then.

kgoderis commented 8 years ago

I could not find any info about : Miele, Helios and ATSAdvanced.

Miele and Helios are bindings that are still (@kaikreuzer ahum ;-) ) in a (OH2) PR. ATSAdvanced is a binding that I can not publish due to SDK license constraints

paphko commented 8 years ago

There is already an issue for the FS Internet Radio binding: https://github.com/eclipse/smarthome/issues/682 but AFAIK there is no work done for it yet.

svilenvul commented 8 years ago

Bundles using package org.eclipse.smarthome.io.net.http from org.eclipse.smarthome.io.net (that currently depends on Apache HttpClient 3.x):

openhab2-addons\addons\binding\org.openhab.binding.astro\META-INF\MANIFEST.MF openhab2-addons\addons\binding\org.openhab.binding.avmfritz\META-INF\MANIFEST.MF openhab2-addons\addons\binding\org.openhab.binding.hdanywhere\META-INF\MANIFEST.MF smarthome\bundles\model\org.eclipse.smarthome.model.script\META-INF\MANIFEST.MF smarthome\bundles\ui\org.eclipse.smarthome.ui\META-INF\MANIFEST.MF smarthome\extensions\binding\org.eclipse.smarthome.binding.sonos\META-INF\MANIFEST.MF smarthome\extensions\binding\org.eclipse.smarthome.binding.wemo\META-INF\MANIFEST.MF smarthome\extensions\ui\org.eclipse.smarthome.ui.basic\META-INF\MANIFEST.MF

svilenvul commented 8 years ago

kaikreuzer wrote:

Please ONLY regard the bundles in smarthome and openhab2-addons repository

I found some missing dependencies in the SmartHome target platform:

[DEBUG] No solution found because the problem is unsatisfiable.: [Unable to satisfy dependency from org.eclipse.smarthome.io.net 0.9.0.qualifier to package org.
eclipse.jetty.client 0.0.0.; Unable to satisfy dependency from org.eclipse.smarthome.io.net 0.9.0.qualifier to package org.eclipse.jetty.client.api 0.0.0.; Unab
le to satisfy dependency from org.eclipse.smarthome.io.net 0.9.0.qualifier to package org.eclipse.jetty.client.util 0.0.0.; No solution found because the proble
m is unsatisfiable.]

In openHAB target platform is located in openhab-deps-repo:

[INFO] Fetching org.eclipse.jetty.client_9.2.12.v20150709.jar from https://dl.bintray.com/openhab/p2/openhab-deps-repo/1.0.6/plugins/ (0B of 246,93kB at 0B/s)
kaikreuzer commented 8 years ago

@svilenvul Just add this commit to your PR and the Jetty client is part of the smarthome.target: https://github.com/kaikreuzer/smarthome/commit/56d8ab652ebbcc189e1c62170473a073b9772172

svilenvul commented 8 years ago

@kaikreuzer thanks