Feature request: Switch HTTP client library from Apache HttpClient 4.x to Apache HttpClient 5.x (or okhttp / etc...)

[JellyBrick]

Apache HttpClient 5.x and okhttp (or ktor) support HTTP/2 (and asynchronous HTTP requests).

Switching to these libraries is expected to enable more efficient HTTP requests.

Migrate issue from sedmelluq/lavaplayer#652

[Walkyst]

Feel free to do PR

[jocull]

Is there a major benefit to doing this? Is the current version causing any problems?

[JellyBrick]

@jocull

Is there a major benefit to doing this? Is the current version causing any problems?

HttpClient has the problem of being overly complex and requires writing tons of class implementation. This issue makes it difficult to resolve issues like #86.

In addition, HttpClient 4 does not support HTTP 2.0. This is not appropriate for lavaplayer where video streams need to be downloaded continuously.

[EncryptSL]

This need very great knowledge of changes between 4x and 5x.

[jocull]

I did take a look at this there are two things worth noting:

1. Many of the public interfaces accept specific configurations or builders and then seems to use them throughout the library. Because of the public facing nature of the client, this upgrade is a breaking change that will require other uses to update their code to use and configure for the new HTTP library as well.
2. The scope of work is relatively large, requiring something like 50 classes to be changed.

I haven't been able to scope or examine the changes further. I think at this point the value should be questioned again since it's a major change for not just the library itself, but everyone that depends on it 🙂

[JellyBrick]

Agree, I think it can cause a lot of breaking changes.

Pros:

• lavaplayer's complex HttpClient setup has caused memory leaks and other bugs in the past, I think moving to a simpler HTTP client library will help with maintenance.
  (sedmelluq/lavaplayer@0480e1a)
  (Lavaplayer not closing connections when using BalancingIpRoutePlanner with ipv6 sedmelluq/lavaplayer#514)
  (Memory leaked (Socket connection leaked) #86)

• Can easily configure new features such as Brotli, cache, etc.

Cons:

• It requires a large amount of change (over 50 classes)
• Can break third-party libraries that rely on lavaplayer caused by breaking changes

[jocull]

I skimmed some of the issues. I can't tell how much of the issue is from user provided misconfiguration vs the library is not cleaning up after itself.

I think I would personally make the goal to make the library harder to misconfigure (and fix any obvious bugs within it like not closing connections that should be closed). Those can be treated separately.

Perhaps instead of exposing implementation to specific HttpBuilders for example, expose a number of tunable properties. The max number of connections, the max time to hold a connection open, etc. Maybe there's ways to improve specific issues people are having before a wholesale library change?