Skip to content

Commit

Permalink
Fix NetworkingModule losing Cookies when multiple CatalystInstances e…
Browse files Browse the repository at this point in the history 
…xist and one is destroyed

Differential Revision: D4451197

fbshipit-source-id: 905309f626a2295ecaa2451413e414eb827e10b0
  • Loading branch information
@olegbl @facebook-github-bot
olegbl authored and facebook-github-bot committed Feb 3, 2017
1 parent f4bbf1b commit 0a71f48
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 5 deletions.
7 changes: 3 additions & 4 deletions ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ public final class NetworkingModule extends ReactContextBaseJavaModule {
client = clientBuilder.build();
}
mClient = client;
OkHttpClientProvider.replaceOkHttpClient(client);
mCookieHandler = new ForwardingCookieHandler(reactContext);
mCookieJarContainer = (CookieJarContainer) mClient.cookieJar();
mShuttingDown = false;
Expand All @@ -113,7 +112,7 @@ public final class NetworkingModule extends ReactContextBaseJavaModule {
* @param context the ReactContext of the application
*/
public NetworkingModule(final ReactApplicationContext context) {
this(context, null, OkHttpClientProvider.getOkHttpClient(), null);
this(context, null, OkHttpClientProvider.createClient(), null);
}

/**
Expand All @@ -124,7 +123,7 @@ public NetworkingModule(final ReactApplicationContext context) {
public NetworkingModule(
ReactApplicationContext context,
List<NetworkInterceptorCreator> networkInterceptorCreators) {
this(context, null, OkHttpClientProvider.getOkHttpClient(), networkInterceptorCreators);
this(context, null, OkHttpClientProvider.createClient(), networkInterceptorCreators);
}

/**
Expand All @@ -133,7 +132,7 @@ public NetworkingModule(
* caller does not provide one explicitly
*/
public NetworkingModule(ReactApplicationContext context, String defaultUserAgent) {
this(context, defaultUserAgent, OkHttpClientProvider.getOkHttpClient(), null);
this(context, defaultUserAgent, OkHttpClientProvider.createClient(), null);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ReactAndroid/src/main/java/com/facebook/react/modules/network/OkHttpClientProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ public static void replaceOkHttpClient(OkHttpClient client) {
sClient = client;
}

private static OkHttpClient createClient() {
public static OkHttpClient createClient() {
// No timeouts by default
OkHttpClient.Builder client = new OkHttpClient.Builder()
.connectTimeout(0, TimeUnit.MILLISECONDS)
Expand Down

19 comments on commit 0a71f48

@thotegowda
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @olegbl , We were using OkHttpClientProvider.replaceOkHttpClient() to substitute our custom OkHttpClient (from our brown-field app) to manage interceptors, headers etc. With this commit, I don't see any way to substitute our custom OkHttpClient.

Is there any way to achieve the same?

@olegbl
Copy link
Contributor Author

@olegbl olegbl commented on 0a71f48 Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OkHttpClientProvider.replaceOkHttpClient() didn't go anywhere, it's still usable. It's just no longer used in NetworkingModule.

Or, do you mean that you were replacing the client inside of NetworkingModule using that? If so, I don't think there's a way right now. You'd have to extend OkHttpClientProvider or NetworkingModule to allow that sort of thing (but please don't make it use a single instance for everything as before, it would need to replace it in a way where you define how it's created rather than give it an instance.)

@thotegowda
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not being clear,

Before this commit, NetworkingModule would call get OkHttpClientProvider.getOkHttpClient() in the constructors, so if calling OkHttpClientProvider.replaceOkHttpClient() during app startup with custom OkHttpClient instance would result in NetworkingModule using the custom OkHttpClient instance.

But now after changing getOkHttpClient() to createClient(), this option is eliminated. Even if its set using replaceOkHttpClientI(), NetworkingModule is not going to be picked up.

I'm not able to find any way to get this working now because (1) can't extend OkHttpClientProvider, it has only static method. (2) can't extend NetworkingModule, it is final class.

Am I missing something here ?

@olegbl
Copy link
Contributor Author

@olegbl olegbl commented on 0a71f48 Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, so it's the case I described in my second paragraph. To clarify, by "extend", I meant add new functionality and submit a PR.

@thotegowda
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha! Thanks.

@cenkakin
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @olegbl ,

With this commit, from my module, I lost the ability to access OkHttpClient instance which is being used by RN NetworkModule 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?

@codebymikey
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olegbl, this change breaks our stetho network inspections.

Is there no other away around this? I need a way to set the OkHttpClient which is being passed into the NetworkingModule .

NetworkingModule appears to be a final class and can't be subclassed.

And adding another Module to do that seems unlikely to work, and would most likely cause breaking changes if we ever need to update the react version.

What if we add an interface that allows users to provide their own OkHttpClient based on the context. Something similar to:

// com/facebook/react/modules/network/OkHttpClientProvider.java
//...

interface OkHttpClientProvider {
  OkHttpClient createNewNetworkModuleClient();
}

// Public setter for setting this.
static OkHttpClientProvider provider;

public static OkHttpClient createClient() {
    if(provider != null) {
      // Use a user defined provider.
      return provider.createNewNetworkModuleClient();
    }
    // 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());

    return enableTls12OnPreLollipop(client).build();
  }
//...

@thotegowda
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olegbl Please check this commit, if you have time.
I took a shot at fixing this. If it looks OK, will raise a PR.

@codebymikey
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thotegowda, your commit looks fine to me. Good job.

Only potential blocker might be the breaking name change to setFrescoConfig which I personally think is acceptable since it can be easily fixed.

@thotegowda
Copy link

@thotegowda thotegowda commented on 0a71f48 May 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codebymikey Thanks.

I'm still thinking about setFrescoConfig change. I had changed it as it was unused, but now wondering if its used internally by fb.

@codebymikey
Copy link

@codebymikey codebymikey commented on 0a71f48 May 7, 2017 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leon087
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@olegbl Please check this commit, tks.

// com/facebook/react/modules/network/OkHttpClientProvider.java
//...
public interface IProvider {
    OkHttpClient create();

    OkHttpClient get();
  }

  public static class DefaultProvider implements OkHttpClientProvider.IProvider {
    // Centralized OkHttpClient for all networking requests.
    private static @Nullable OkHttpClient sClient;

    @Override
    public OkHttpClient create() {
      // 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());

      return enableTls12OnPreLollipop(client).build();
    }

    @Override
    public OkHttpClient get() {
      if (sClient == null) {
        sClient = create();
      }
      return sClient;
    }
  }

  private static @Nullable OkHttpClientProvider.IProvider sPprovider;

  private static OkHttpClientProvider.IProvider getProvider() {
    if (sPprovider == null) {
      sPprovider = new DefaultProvider();
    }
    return sPprovider;
  }

  // okhttp3 OkHttpClientProvider.IProvider is immutable
  // This allows app to init an OkHttpClientProvider.IProvider with custom settings.
  public static void replaceProvider(OkHttpClientProvider.IProvider provider) {
    sPprovider = provider;
  }

  public static OkHttpClient getOkHttpClient() {
    return getProvider().get();
  }

  public static OkHttpClient createClient() {
    return getProvider().create();
  }

@celesteshire
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bad！

@tunatoksoz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also limiting the ability to certificate pin, whitelisting self-signed certs, or certificates where browser considers ok but okhttp doesnt (chain validation issue).

@tunatoksoz
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An easy work around is to use reflection to modify the instance Networking module keeps. Not ideal, but solved my problem.

@markshhsu
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tunatoksoz Could you show a sample code for how to use reflection to modify the instance in the Networking module? Thank you!

@tunatoksoz
Copy link

@tunatoksoz tunatoksoz commented on 0a71f48 Nov 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cropped off of my current project:

class RGNetworkingModule extends ReactContextBaseJavaModule {
  private ReactApplicationContext mApplicationContext;
  public RGNetworkingModule(ReactApplicationContext context) {
    super(context);
    mApplicationContext = context;
  }
  @Override
  public String getName() {
      return "RGFakeModule";
  }

  private OkHttpClient getClientForModule(NativeModule module)
    throws Exception {
    Field f = module.getClass().getDeclaredField("mClient");
    f.setAccessible(true);
    return (OkHttpClient)f.get(module);
  }

  private void setClientForModule(
    NativeModule module, OkHttpClient client)
    throws Exception {
    Field f = module.getClass().getDeclaredField("mClient");
    f.setAccessible(true);
    f.set(module, client);
  }

  private NativeModule getNetworkingModule() {
    Collection<NativeModule> modules =
        mApplicationContext.getCatalystInstance().getNativeModules();
    for (NativeModule module : modules) {
      if ("Networking".equals(module.getName())) {
        return module;
      }
    }
    return null;
  }


  @Override
  public void initialize() {
    try {
      NativeModule module = getNetworkingModule();
      OkHttpClient client = getClientForModule(module);

      OkHttpClient newClient = client
        .newBuilder()
        // Do something with cache.
        .build();
      setClientForModule(module, newClient);
    } catch (Exception ex) {
      android.util.Log.e("RG", "Error while patching networking module.", ex);
    }
  }
}

It may or may not work with some of the functionality, so use on your own risk.

@markshhsu
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tunatoksoz Thank you very much.

@sytolk
Copy link

@sytolk sytolk commented on 0a71f48 Nov 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tunatoksoz this module works with RN v 0.49 just its need to add StethoInterceptor

HttpLoggingInterceptor logging = new HttpLoggingInterceptor();
            logging.setLevel(HttpLoggingInterceptor.Level.BODY);

            OkHttpClient newClient = client
                .newBuilder()
                .addNetworkInterceptor(new StethoInterceptor())
                .addInterceptor(logging)
                .build();

Please sign in to comment.