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.

Sign up for GitHub

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

networking: Fix connection corruption after turning network off and on. #2

Open
wants to merge 2 commits into
base: 0.55.4-zulip
Choose a base branch
from
Open

networking: Fix connection corruption after turning network off and on. #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa2e00a
networking: Fix requests failing when turning network off and on #19709.
roberthoenig Jun 22, 2018
4480679
networking: Fully fix requests failing when turning network off and o…
roberthoenig Jun 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions ReactAndroid/src/main/java/com/facebook/react/ReactActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,18 @@

import javax.annotation.Nullable;

import android.Manifest;
import android.app.Activity;
import android.content.Intent;
import android.content.pm.PackageManager;
import android.os.Bundle;
import android.support.v4.content.ContextCompat;
import android.view.KeyEvent;

import com.facebook.react.modules.core.DefaultHardwareBackBtnHandler;
import com.facebook.react.modules.core.PermissionAwareActivity;
import com.facebook.react.modules.core.PermissionListener;
import com.facebook.react.modules.network.OkHttpClientProvider;

/**
* Base Activity for React Native applications.
Expand Down Expand Up @@ -50,6 +54,10 @@ protected ReactActivityDelegate createReactActivityDelegate() {
protected void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
mDelegate.onCreate(savedInstanceState);
if (ContextCompat.checkSelfPermission(this, Manifest.permission.ACCESS_NETWORK_STATE)
== PackageManager.PERMISSION_GRANTED) {
OkHttpClientProvider.addNetworkListenerToEvictIdleConnectionsOnNetworkChange(getApplicationContext());
}
}

@Override
Expand Down
63 changes: 62 additions & 1 deletion ReactAndroid/src/main/java/com/facebook/react/modules/network/OkHttpClientProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,22 @@

package com.facebook.react.modules.network;

import android.content.BroadcastReceiver;
import android.content.Context;
import android.content.Intent;
import android.content.IntentFilter;
import android.net.ConnectivityManager;
import android.net.NetworkInfo;
import android.os.Build;
import android.os.Bundle;

import com.facebook.common.logging.FLog;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Set;
import java.util.WeakHashMap;
import java.util.concurrent.TimeUnit;

import javax.annotation.Nullable;
Expand All @@ -33,6 +43,9 @@ public class OkHttpClientProvider {
// User-provided OkHttpClient factory
private static @Nullable OkHttpClientFactory sFactory;

private final static Set<OkHttpClient> sClients = Collections.newSetFromMap(
new WeakHashMap<OkHttpClient, Boolean>());

public static void setOkHttpClientFactory(OkHttpClientFactory factory) {
sFactory = factory;
}
Expand All @@ -43,6 +56,48 @@ public static OkHttpClient getOkHttpClient() {
}
return sClient;
}

/*
See https://github.com/facebook/react-native/issues/19709 for context.
We know that connections get corrupted when the connectivity state
changes to disconnected, but the debugging of this issue hasn't been
exhaustive and it's possible that other changes in connectivity also
corrupt idle connections. `CONNECTIVITY_ACTION`s occur infrequently
enough to go safe and evict all idle connections and ongoing calls
for the events DISCONNECTED and CONNECTING. Don't do this for CONNECTED
since it's possible that new calls have already been dispatched by the
time we receive the event.
*/
public static void addNetworkListenerToEvictIdleConnectionsOnNetworkChange(Context context) {
final BroadcastReceiver br = new BroadcastReceiver() {
@Override
public void onReceive(Context context, Intent intent) {
if (!intent.getAction().equals(ConnectivityManager.CONNECTIVITY_ACTION)) {
return;
}
final Bundle extras = intent.getExtras();
final NetworkInfo info = extras.getParcelable("networkInfo");
final NetworkInfo.State state = info.getState();
if (state == NetworkInfo.State.CONNECTED) {
return;
}
final PendingResult result = goAsync();
final Thread thread = new Thread() {
public void run() {
for (OkHttpClient client: sClients) {
client.dispatcher().cancelAll();
client.connectionPool().evictAll();
}
result.finish();
}
};
thread.start();
}
};
final IntentFilter intentFilter = new IntentFilter();
intentFilter.addAction(ConnectivityManager.CONNECTIVITY_ACTION);
context.registerReceiver(br, intentFilter);
Copy link
Member

Choose a reason for hiding this comment

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

I'm looking at the doc for CONNECTIVITY_ACTION:
https://developer.android.com/reference/android/net/ConnectivityManager#CONNECTIVITY_ACTION

The doc says this is deprecated and there are better APIs -- but those require API level 24. So that's not happening soon.

Also, it looks like we'll get this if one network connection drops, even if there are others or if the system is attempting to connect to another. For this usage, I think that's fine. ... Oh, I see, the DISCONNECTED is a level-triggered state, not an edge-triggered event:
https://developer.android.com/reference/android/net/NetworkInfo.State
and if we're connecting to some other network, it'll be CONNECTING instead.

I wonder if we should do this also on CONNECTING -- for example, we might go CONNECTED -> CONNECTING -> CONNECTED, having switched to a different network, and the same bug will probably appear. The simple version would end up evicting twice in the case of CONNECTED -> DISCONNECTED -> CONNECTING -> CONNECTED, but that's probably fine -- there shouldn't be tons of clients, and evicting an empty connection pool on each one ought to be very cheap.

I'm also curious what SUSPENDED means, and wonder if we should be evicting on that too. In which case it's just state != NetworkInfo.State.CONNECTED.

In any case, I think a comment is in order explaining the logic here and the choices about when to do the eviction -- that will be helpful for anyone trying to understand it, including (a) a reviewer (me, and someone upstream), (b) someone debugging it in the future 😉 , (c) someone porting it in maybe 2023 to use the spiffy new APIs from today's recent Android versions. Maybe a block comment right above this function?

Copy link
Author

Choose a reason for hiding this comment

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

I'm also curious what SUSPENDED means, and wonder if we should be evicting on that too. In which case it's just state != NetworkInfo.State.CONNECTED.

According to this SO answer:

The connected state indicates that your phone is connected and should be able to access IP traffic. The suspended state indicates that your IP traffic is temporarily unavailable but you are still connected.
An example from the TelephonyManager is when you have access to a 2G network and you receive a phone call, the data traffic may be suspended.

I think we should opt for the state != NetworkInfo.State.CONNECTED. IP traffic unavailable -> possible interference with okhttp. Also, evicting too often isn't nearly as problematic as not evicting often enough, and given the above description, SUSPEND shouldn't occur often.

Copy link
Author

Choose a reason for hiding this comment

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

Actually we could just trigger this on any NetworkInfo change event, that is, including CONNECT. After all, we don't know yet exactly which event causes okhttp to break. In my tests, it worked fine just evicting on DISCONNECT, but that was in my isolated emulator environment.

}

// okhttp3 OkHttpClient is immutable
// This allows app to init an OkHttpClient with custom settings.
Expand All @@ -54,7 +109,9 @@ public static OkHttpClient createClient() {
if (sFactory != null) {
return sFactory.createNewNetworkModuleClient();
}
return createClientBuilder().build();
final OkHttpClient client = createClientBuilder().build();
registerClientToEvictIdleConnectionsOnNetworkChange(client);
return client;
}

public static OkHttpClient.Builder createClientBuilder() {
Expand All @@ -68,6 +125,10 @@ public static OkHttpClient.Builder createClientBuilder() {
return enableTls12OnPreLollipop(client);
}

public static void registerClientToEvictIdleConnectionsOnNetworkChange(OkHttpClient client) {
sClients.add(client);
}

/*
On Android 4.1-4.4 (API level 16 to 19) TLS 1.1 and 1.2 are
available but not enabled by default. The following method
Expand Down