Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom OkHttpClient #68

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

vyndor
Copy link
Contributor

@vyndor vyndor commented Oct 6, 2023

Description

Adds ability to pass custom OkHttpClient thus making it easy to configure any network parameters for centrifuge. Some examples:

  • DNS, proxy etc
  • Reuse OkHttpClient instance that app is already using
  • Reuse application thread pool
  • Custom networking logic in interceptors (our case, will write it down below)

On the downside - centrifuge exposes okhttp API making it harder to change networking library later.

What's been done

  • Add new methods getOkHttpClient/setOkHttpClient to Options class
  • Deprecate setProxyCredentials and setDns because now it can be configured in OkHttpClient

Why our project needs this

Our project needs an ability to modify endpoint in case of specific errors:

  • UnknownHostException, SocketTimeoutException
  • Some subset of HTTP response codes. For example 410

We already have this logic in interceptor for okhttp. So it would be convinient to just reuse this interceptor for centrifuge.

@mnagovitsin
Copy link

Hello

Me and @FZambia started a discussion about these changes in Telegram.

As @FZambia said this PR is not going to be merged right now to avoid introducing hard dependency on OkHttp.

I suggested following approach to avoid hard dependency:
Options.java

public void setCustomClient(Object httpClient) {
     // some runtime checks on `httpClient.getClass().getName()` to assure that we can work with such client, right now it is only an OkHttpClient
}

But it introduces runtime checks which is not really nice way to solve this.

Right now we are trying decide if it is possible to provide customization of http client itself or may be it is not necessary.

The initial problem that our team is trying to solve is strict requirements from our architecture committee on libraries that are candidates to integrate into our Android application.

One of is requirements is the ability to reuse existing http client (reasons are the same as in the description of that PR: reuse application thread pool, custom networking logic in interceptors, custom DNS and proxy settings).

@FZambia
Copy link
Member

FZambia commented Aug 22, 2024

I suggested following approach to avoid hard dependency

My concern was that centrifuge-java won't be able to switch from OKHttp or migrate to another version of OKHttp in that case without potentially breaking your app code. And the proposed solution with Object does not fix this I think.

One of is requirements is the ability to reuse existing http client (reasons are the same as in the description of that PR: reuse application thread pool, custom networking logic in interceptors, custom DNS and proxy settings).

It's now possible to set Proxy, DNS.

What is the custom logic of network interceptors you need? Probably it may be solved by #69 ?

Regarding reusing thread pool – I don't have understanding how critical this is? Does it bring a lot of overhead now? I believe thread pool may grow on demand internally (though it's just an assumption), so it may be no real overhead at the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants