React-native: Accessing same OkHttpClient used all networking requests is broken after 0.43.0

[cenkakin]

Hi,

After commit 0a71f48b1349ed09bcb6e76ba9ff8eb388518b15, from my module, I lost the ability to access OkHttpClient instance which is being used in entire network requests provided via OkHttpClientProvider. I need this to automatically load cookies stored by RN network requests to the network requests in my module. Is there any other way to do that since this way is not working after this commit? Or maybe you will provide another one?

[hey99xx]

@olegbl Honestly this commit should be probably reverted and whatever's the problem with cookies should be fixed properly. This breaks us as well. In our brown-field app we already create multiple instances of ReactInstanceManager each associated with one NetworkingModule.

At the startup of the app, we were able to inject our own cookie handler and network interceptors. With this change, there's going to be a new OkHttp client created from scratch in each networking module. And correct me if I am wrong, but now variable sClient is unused by React native until someone manually calls OkHttpClientProvider#getOkHttpClient method, which makes this variable practically obsolete. The comment on top of the variable // Centralized OkHttpClient for all networking requests. is also entirely inaccurate.

[hey99xx]

This commit also looks potentially buggy https://github.com/facebook/react-native/commit/d2796ea4ed8ed7fb4d5a2c4ae3f73fbd2ced2c51 if creating a custom OkHttpClient will just for images will remove my custom interceptors. For example how will I be able to inject a different user agent to my image download requests if my server needs to determine which image size to use?

[olegbl]

Feel free to fix this the right way. I'm not a RN developer, so I don't know what that would be. There was just a crash that broke a ton of things and no RN dev had time, so I had to do it.

The issue this commit (0a71f48b1349ed09bcb6e76ba9ff8eb388518b15) fixed though was not a "this is not possible to do (missing feature)" but a "everything breaks down (issue with existing feature)" kind of issue. So, whatever the fix, just make sure it doesn't regress.

Just remember NetworkingModule totally fudges up the OkHttp client it's using when it's being cleaned up via a CatalystInstance's destructor (see onCatalystInstanceDestroy). So, if you ever use the same OkHttp client elsewhere, you're going to have a bad time. I imagine you'll want to extend OkHttpClientProvider to allow overriding the builder in some way.

The comment on top of the variable is also entirely inaccurate.

I agree! Nobody I asked knows why in the world this shared module was ever made in the first place.

BTW, d2796ea fixes the same issue but for images. When a dying catalystinstance destroys the cookiejar of the cached okhttpclient, that breaks anything using it - hence the image stuff also needed its own instance.

[hey99xx]

Alright, I apologize if my first message started a bit harsh. If it was crashing, I agree that an immediate but imperfect fix is better than having no fix anyway. I will try to read the entire OkHttp section of React later to understand its issues. For example, I don't even know why we would need to destroy the cookiejar anyways. Why don't let it be and keep sharing in multiple network modules?

[olegbl]

Why don't let it be and keep sharing in multiple network modules?

I think this was done to fix some kind of memory leak (https://github.com/facebook/react-native/commit/3a00545bc77c74d4fd49584e262326d3dffabb2f). I don't know the details though.

[hey99xx]

@olegbl Would it make sense to open this constructor to public https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java#L104 or https://github.com/facebook/react-native/blob/master/ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java#L76 instead of package-private /* package */ so people who want to maintain the same networking module can still do so without overriding NetworkModule completely.

[Knight704]

I have a solution for current situation, which is aimed to customize okhttpclient.
This is based on fact that NetworkingModule has constructors which accepts okhttpclient, unfortunately (like @hey99xx said they are package private, but it's easy to create package with the same name 'facebook.react.modules.network' to relax it's access).

In a nutshell it boils down to:
1) Create class that extends MainReactPackage, and pass it instead MainReactPackage in MainApplication.java

2) Override method getNativeModules and then replace Networking provider with custom one.

public class MyMainReactPackage extends MainReactPackage {
    @Override
    public List<ModuleSpec> getNativeModules(ReactApplicationContext context) {
        List<ModuleSpec> nativeModules = super.getNativeModules(context);
        return adjustModules(context, nativeModules);
    }

    private List<ModuleSpec> adjustModules(ReactApplicationContext context, List<ModuleSpec> moduleSpecs) {
        ArrayList<ModuleSpec> modules = new ArrayList<>(moduleSpecs);

        for (int i = 0; i < modules.size(); i++) {
            ModuleSpec spec = modules.get(i);
            if (spec.getType().equals(NetworkingModule.class)) {
                modules.set(i, getCustomNetworkingModule(context));
                break;
            }
        }

        return modules;
    }

    private ModuleSpec getCustomNetworkingModule(final ReactApplicationContext context) {
        return new ModuleSpec(NetworkingModule.class, new Provider<NativeModule>() {
            @Override
            public NativeModule get() {
                return NetworkingModuleUtils.createNetworkingModuleWithCustomClient(context);
            }
        });
    }
}

Here my NetworkingModuleUtils.java has full path of com.facebook.react.modules.network.NetworkingModuleUtils and look like:

public class NetworkingModuleUtils {
    public static NetworkingModule createNetworkingModuleWithCustomClient(ReactApplicationContext context) {
        OkHttpClient client = OkHttpClientProvider.createClient();
        // ... full access to customize client
        return new NetworkingModule(context, null, customClient);
    }
}

[celesteshire]

This problem still exists, looking forward to the official repair

[Jobeso]

+1

[Kottidev]

+1

[aaronbuchanan]

+1

[ada-lovecraft]

+1

[WangLaoSeng]

+1

[kohei-takata]

+1

[team55]

+1

[byteninja]

+1

[tillh]

+1

[odemolliens]

+1

[hey99xx]

Still +1

[hey99xx]

I found that NetworkingModule is a final class, so I cannot even extend it. Will try @Knight704 's solution now...

[alexwmidas]

@Knight704 actually, I had a same problem and I implemented your method.

[alexwmidas]

@Knight704 but I have a problem with making the build.

Error:Error converting bytecode to dex:
Cause: com.android.dex.DexException: Multiple dex files define Lcom/facebook/react/modules/network/NetworkingModule$1$1;
Error:com.android.dex.DexException: Multiple dex files define Lcom/facebook/react/modules/network/NetworkingModule$1$1;
Error:  at com.android.dx.merge.DexMerger.readSortableTypes(DexMerger.java:661)
Error:  at com.android.dx.merge.DexMerger.getSortedTypes(DexMerger.java:616)
Error:  at com.android.dx.merge.DexMerger.mergeClassDefs(DexMerger.java:598)
Error:  at com.android.dx.merge.DexMerger.mergeDexes(DexMerger.java:171)
Error:  at com.android.dx.merge.DexMerger.merge(DexMerger.java:198)
Error:  at com.android.builder.dexing.DexArchiveMergerCallable.call(DexArchiveMergerCallable.java:61)
Error:  at com.android.builder.dexing.DexArchiveMergerCallable.call(DexArchiveMergerCallable.java:36)
Error:  at java.util.concurrent.ForkJoinTask$AdaptedCallable.exec(ForkJoinTask.java:1424)
Error:  at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:289)
Error:  at java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1056)
Error:  at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1692)
Error:  at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)
Error:Execution failed for task ':app:transformDexArchiveWithDexMergerForDebug'.
> com.android.build.api.transform.TransformException: com.android.dex.DexException: Multiple dex files define Lcom/facebook/react/modules/network/NetworkingModule$1$1;

Here is the screenshot of my project. Did I make it wrong?

@Knight704 How can I resolve this issue?

[shokimble]

Should @Knight704 's solution still work on 0.51 of React Native? I've implemented it but I get an error like below:

MyMainReactPackage.java:38: error: ModuleSpec(Class,Provider) has private access in ModuleSpec
return new ModuleSpec(NetworkingModule.class, new Provider() {

I've implemented it as suggested and using the example NetworkingModuleUtils.java placed in com/facebook/react/modules/network. Below:

NetworkingModuleUtils.java
https://gist.github.com/shokimble/ff969668844399321a049c7e0f1351da

MyMainReactPackage.java
https://gist.github.com/shokimble/f735b19538d3cf47f13a49cde94e4ba2

And two lines in MainApplication.java to import MyMainReactPackage.java and use it instead of MainReactPackage

Any clue as to what I have done wrong? It looks to me like something is now private that wasn't previously but going back over the react native codebase I am at a loss to work it out (too much to go through).

[odemolliens]

@shokimble do you have put the NetworkingModuleUtils in the good package?

com.facebook.react.modules.network

[shokimble]

@odemolliens it has

package com.facebook.react.modules.network;

at the top of the file and it's in the directory src/main/assets/java/com/facebook/react/modules/network so yes is the answer.

I'm not that strong with java but I believe that's all that is necessary.

[hey99xx]

@shokimble you are right. ModuleSpec has been made private in this commit https://github.com/facebook/react-native/commit/f0fb720eaa6a09f102ef61cfa6083c1f254f1edc#diff-d4ca0976202f1b023e5905b47453e98f

the new syntax seems to be

ModuleSpec.nativeModuleSpec(NetworkingModule.class, new Provider<NativeModule>() {
    @Override
    public NativeModule get() {
        return NetworkingModuleUtils.createNetworkingModuleWithCustomClient(context);
    }
})

In any case, I see this PR being merged in https://github.com/facebook/react-native/commit/22efd95be1f0b236eeaaa8a8e6d01e89771c9543 so in next release there will be a more preferable way without touching MainReactPackage at any point.

[hey99xx]

See https://github.com/facebook/react-native/commit/f0fb720eaa6a09f102ef61cfa6083c1f254f1edc#diff-884b8b13dc59cfaab62da61f4596f821L232 for the exact change from

new ModuleSpec(

to

ModuleSpec.nativeModuleSpec(.

That's literally all you'll have to change from @Knight704 's solution (btw it works great for me 👍 )

[hey99xx]

If any FB engineer comes to this issue later, once you have a release containing https://github.com/facebook/react-native/commit/22efd95be1f0b236eeaaa8a8e6d01e89771c9543 I think it should be safe to close this issue for good.

[shokimble]

Perfect! Thanks for tracking that down @hey99xx

[hramos]

https://github.com/facebook/react-native/commit/22efd95be1f0b236eeaaa8a8e6d01e89771c9543 is part of 0.54.