Add ability to customise WebSocket OkHttp client on Android

[tolbkni]

Motivation

Summary:
Prior to 22efd95, users could customise the OkHttp client used by React Native on Android by calling setOkHttpClientFactory in OkHttpClientProvider.

This functionality has a variety of legitimate applications from changing connection timeouts or pool size to Stetho integration. In this commit also provide the ability for WebSocket.

Closes #18920

Test Plan

Create React Native application and set a custom factory in the constructor, e.g. OkHttpClientProvider.setOkHttpClientFactory(new CustomNetworkModule());

Where a custom factory would look like:

class CustomNetworkModule implements OkHttpClientFactory {
    public OkHttpClient createNewNetworkModuleClient() {
        return new OkHttpClient.Builder().build();
    }
}

Related PRs

None

Release Notes

[ANDROID] [MINOR] [WebSocket] - Also use the custom factory set in OkHttpClientProvider for customising the WebSocket OkHttp client used by React Native

[hey99xx]

Do you think there's value moving creation of the OkHttp client into the constructor of WebSocketModule? Right now it looks to me like it creates a new client for every new websocket connection, which looks expensive.

[tolbkni]

Sounds good idea, here I just change the way to create OkHttp client, to provider ability for WebSocket. And maybe we can create another PR to improve.

---- updated ----
@hey99xx I consider that WebSocket is generally used for long connection, clients are not frequently created, so it seems that this problem does not so bad.

[facebook-github-bot]

@tolbkni I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

[tolbkni]

Please review and merge @hramos @mdvacca.

[boliveira]

Hi, I need this fix to be available so we can use certificate pinning on websocket connections!!! Please, fix this asap facebook!!!

[hramos]

The change seems fine to me, but I'd like some clarification on the default behavior when a custom OKHttpClient is not passed to the constructor.

[cpojer]

It doesn't seem like this is the right change as it changes the defaults. Can you make it so the default behavior stays?

[tolbkni]

@cpojer @hramos I have updated the code to stay the default behavior.

[cpojer]

Thank you!

[facebook-github-bot]

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[boliveira]

Hello,

I expected that by release 0.59 this would land. Why this hasn't landed yet? This makes upgrading to newer versions of react native harder for us as we have implemented it in a fork.

[hramos]

There's a lot of work going on to get 0.59 and future releases ready. This one missed the 0.59 release, but the good news is that 0.60 and 0.61 are around the corner. Please avoid adding unnecessary pressure to maintainers! We'd appreciate it.

[boliveira]

Hi Hector, I am sorry for the language used. I love react native and the work you do.
It's just that without this change I am not able to upgrade react native to newer versions without doing a lot of work and as time goes on, more and more versions keep getting released and I fear I'll not be able to keep the pace.

[cpojer]

It seems like this diff got stuck when I tried to land it :(

[facebook-github-bot]

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[cpojer]

I've been trying to land this for a while at FB and it fails in the apps that rely on this. Does this change come with any actual behavior changes that are not obvious?

[boliveira]

Maybe because the defaults are different for Websockets and fetchs.
Maybe a different implementation should be chosen, that would differentiate if the okhttpclient to be created is for use on a websocket or a normal fetch request... just a suggestion...

[tolbkni]

So what can I do? The default way of OkHttpClientProvider.java to create OkHttpClient.Builder is

// No timeouts by default
OkHttpClient.Builder client = new OkHttpClient.Builder()
  .connectTimeout(0, TimeUnit.MILLISECONDS)
  .readTimeout(0, TimeUnit.MILLISECONDS)
  .writeTimeout(0, TimeUnit.MILLISECONDS)
  .cookieJar(new ReactCookieJarContainer());

try {
  Class ConscryptProvider = Class.forName("org.conscrypt.OpenSSLProvider");
  Security.insertProviderAt((Provider) ConscryptProvider.newInstance(), 1);
  return client;
} catch (Exception e) {
  if (Build.VERSION.SDK_INT <= Build.VERSION_CODES.KITKAT) {
    try {
      client.sslSocketFactory(new TLSSocketFactory());

      ConnectionSpec cs = new ConnectionSpec.Builder(ConnectionSpec.MODERN_TLS)
              .tlsVersions(TlsVersion.TLS_1_2)
              .build();

      List<ConnectionSpec> specs = new ArrayList<>();
      specs.add(cs);
      specs.add(ConnectionSpec.COMPATIBLE_TLS);
      specs.add(ConnectionSpec.CLEARTEXT);

      client.connectionSpecs(specs);
    } catch (Exception exc) {
      FLog.e("OkHttpClientProvider", "Error while enabling TLS 1.2", exc);
    }
  }
  return client;
}

[cpojer]

I'm going to close this as we cannot land it without breaking changes as is. If you can find a way to change this PR not to cause breaking changes by keeping the existing behavior and allowing it to be modified I'm happy to reopen it (or feel free to send a new PR).

[insytes]

I've been trying to land this for a while at FB and it fails in the apps that rely on this. Does this change come with any actual behavior changes that are not obvious?

@cpojer Do you have any more information on what's causing it to crash?

[sxlvalue]

Oops, I also encountered the same problem and couldn't find a solution. There is a problem with the basic network connection components of React Native. Is there any way to fix and solve this problem?