diff --git a/library/src/main/java/com/bumptech/glide/manager/SingletonConnectivityReceiver.java b/library/src/main/java/com/bumptech/glide/manager/SingletonConnectivityReceiver.java index 5ebd86ac06..3a56f9029f 100644 --- a/library/src/main/java/com/bumptech/glide/manager/SingletonConnectivityReceiver.java +++ b/library/src/main/java/com/bumptech/glide/manager/SingletonConnectivityReceiver.java @@ -6,14 +6,21 @@ import android.content.Intent; import android.content.IntentFilter; import android.net.ConnectivityManager; +import android.net.ConnectivityManager.NetworkCallback; +import android.net.Network; import android.net.NetworkInfo; +import android.os.Build; +import android.os.Build.VERSION_CODES; import android.util.Log; import androidx.annotation.GuardedBy; import androidx.annotation.NonNull; +import androidx.annotation.RequiresApi; import androidx.annotation.VisibleForTesting; import com.bumptech.glide.manager.ConnectivityMonitor.ConnectivityListener; -import com.bumptech.glide.util.Preconditions; +import com.bumptech.glide.util.GlideSuppliers; +import com.bumptech.glide.util.GlideSuppliers.GlideSupplier; import com.bumptech.glide.util.Synthetic; +import com.bumptech.glide.util.Util; import java.util.ArrayList; import java.util.HashSet; import java.util.List; @@ -22,41 +29,9 @@ /** Uses {@link android.net.ConnectivityManager} to identify connectivity changes. */ final class SingletonConnectivityReceiver { private static volatile SingletonConnectivityReceiver instance; - @Synthetic static final String TAG = "ConnectivityMonitor"; - // Only accessed on the main thread. - @Synthetic boolean isConnected; - - private final BroadcastReceiver connectivityReceiver = - new BroadcastReceiver() { - @Override - public void onReceive(@NonNull Context context, Intent intent) { - List listenersToNotify = null; - boolean wasConnected = isConnected; - isConnected = isConnected(context); - if (wasConnected != isConnected) { - if (Log.isLoggable(TAG, Log.DEBUG)) { - Log.d(TAG, "connectivity changed, isConnected: " + isConnected); - } - - synchronized (SingletonConnectivityReceiver.this) { - listenersToNotify = new ArrayList<>(listeners); - } - } - // Make sure that we do not hold our lock while calling our listener. Otherwise we could - // deadlock where our listener acquires its lock, then tries to acquire ours elsewhere and - // then here we acquire our lock and try to acquire theirs. - // The consequence of this is that we may notify a listener after it has been - // unregistered in a few specific (unlikely) scenarios. That appears to be safe and is - // documented in the unregister method. - if (listenersToNotify != null) { - for (ConnectivityListener listener : listenersToNotify) { - listener.onConnectivityChanged(isConnected); - } - } - } - }; + private static final String TAG = "ConnectivityMonitor"; - private final Context context; + private final FrameworkConnectivityMonitor frameworkConnectivityMonitor; @GuardedBy("this") @Synthetic @@ -69,7 +44,7 @@ static SingletonConnectivityReceiver get(@NonNull Context context) { if (instance == null) { synchronized (SingletonConnectivityReceiver.class) { if (instance == null) { - instance = new SingletonConnectivityReceiver(context); + instance = new SingletonConnectivityReceiver(context.getApplicationContext()); } } } @@ -81,8 +56,34 @@ static void reset() { instance = null; } - private SingletonConnectivityReceiver(@NonNull Context context) { - this.context = context.getApplicationContext(); + private SingletonConnectivityReceiver(final @NonNull Context context) { + GlideSupplier connectivityManager = + GlideSuppliers.memorize( + new GlideSupplier() { + @Override + public ConnectivityManager get() { + return (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE); + } + }); + ConnectivityListener connectivityListener = + new ConnectivityListener() { + @Override + public void onConnectivityChanged(boolean isConnected) { + List toNotify; + synchronized (SingletonConnectivityReceiver.this) { + toNotify = new ArrayList<>(listeners); + } + for (ConnectivityListener listener : toNotify) { + listener.onConnectivityChanged(isConnected); + } + } + }; + + frameworkConnectivityMonitor = + Build.VERSION.SDK_INT >= Build.VERSION_CODES.N + ? new FrameworkConnectivityMonitorPostApi24(connectivityManager, connectivityListener) + : new FrameworkConnectivityMonitorPreApi24( + context, connectivityManager, connectivityListener); } synchronized void register(ConnectivityListener listener) { @@ -106,18 +107,7 @@ private void maybeRegisterReceiver() { if (isRegistered || listeners.isEmpty()) { return; } - isConnected = isConnected(context); - try { - // See #1405 - context.registerReceiver( - connectivityReceiver, new IntentFilter(ConnectivityManager.CONNECTIVITY_ACTION)); - isRegistered = true; - } catch (SecurityException e) { - // See #1417, registering the receiver can throw SecurityException. - if (Log.isLoggable(TAG, Log.WARN)) { - Log.w(TAG, "Failed to register", e); - } - } + isRegistered = frameworkConnectivityMonitor.register(); } @GuardedBy("this") @@ -126,31 +116,167 @@ private void maybeUnregisterReceiver() { return; } - context.unregisterReceiver(connectivityReceiver); + frameworkConnectivityMonitor.unregister(); isRegistered = false; } - @SuppressWarnings("WeakerAccess") - @Synthetic - // Permissions are checked in the factory instead. - @SuppressLint("MissingPermission") - boolean isConnected(@NonNull Context context) { - ConnectivityManager connectivityManager = - Preconditions.checkNotNull( - (ConnectivityManager) context.getSystemService(Context.CONNECTIVITY_SERVICE)); - NetworkInfo networkInfo; - try { - networkInfo = connectivityManager.getActiveNetworkInfo(); - } catch (RuntimeException e) { - // #1405 shows that this throws a SecurityException. - // b/70869360 shows that this throws NullPointerException on APIs 22, 23, and 24. - // b/70869360 also shows that this throws RuntimeException on API 24 and 25. - if (Log.isLoggable(TAG, Log.WARN)) { - Log.w(TAG, "Failed to determine connectivity status when connectivity changed", e); + private interface FrameworkConnectivityMonitor { + boolean register(); + + void unregister(); + } + + @RequiresApi(VERSION_CODES.N) + private static final class FrameworkConnectivityMonitorPostApi24 + implements FrameworkConnectivityMonitor { + + @Synthetic boolean isConnected; + @Synthetic final ConnectivityListener listener; + private final GlideSupplier connectivityManager; + private final NetworkCallback networkCallback = + new NetworkCallback() { + @Override + public void onAvailable(@NonNull Network network) { + postOnConnectivityChange(true); + } + + @Override + public void onLost(@NonNull Network network) { + postOnConnectivityChange(false); + } + + private void postOnConnectivityChange(final boolean newState) { + // We could use registerDefaultNetworkCallback with a Handler, but that's only available + // on API 26, instead of API 24. We can mimic the same behavior here manually by + // posting to the UI thread. All calls have to be posted to make sure that we retain the + // original order. Otherwise a call on a background thread, followed by a call on the UI + // thread could result in the first call running second. + Util.postOnUiThread( + new Runnable() { + @Override + public void run() { + onConnectivityChange(newState); + } + }); + } + + @Synthetic + void onConnectivityChange(boolean newState) { + // See b/201425456. + Util.assertMainThread(); + + boolean wasConnected = isConnected; + isConnected = newState; + if (wasConnected != newState) { + listener.onConnectivityChanged(newState); + } + } + }; + + FrameworkConnectivityMonitorPostApi24( + GlideSupplier connectivityManager, ConnectivityListener listener) { + this.connectivityManager = connectivityManager; + this.listener = listener; + } + + // Permissions are checked in the factory instead. + @SuppressLint("MissingPermission") + @Override + public boolean register() { + isConnected = connectivityManager.get().getActiveNetwork() != null; + try { + connectivityManager.get().registerDefaultNetworkCallback(networkCallback); + return true; + // See b/201664814, b/204226444: At least TooManyRequestsException is not public and + // doesn't extend from any subclass :/. + } catch (RuntimeException e) { + if (Log.isLoggable(TAG, Log.WARN)) { + Log.w(TAG, "Failed to register callback", e); + } + return false; + } + } + + @Override + public void unregister() { + connectivityManager.get().unregisterNetworkCallback(networkCallback); + } + } + + private static final class FrameworkConnectivityMonitorPreApi24 + implements FrameworkConnectivityMonitor { + private final Context context; + @Synthetic final ConnectivityListener listener; + private final GlideSupplier connectivityManager; + @Synthetic boolean isConnected; + + private final BroadcastReceiver connectivityReceiver = + new BroadcastReceiver() { + @Override + public void onReceive(@NonNull Context context, Intent intent) { + boolean wasConnected = isConnected; + isConnected = isConnected(); + if (wasConnected != isConnected) { + if (Log.isLoggable(TAG, Log.DEBUG)) { + Log.d(TAG, "connectivity changed, isConnected: " + isConnected); + } + + listener.onConnectivityChanged(isConnected); + } + } + }; + + FrameworkConnectivityMonitorPreApi24( + Context context, + GlideSupplier connectivityManager, + ConnectivityListener listener) { + this.context = context.getApplicationContext(); + this.connectivityManager = connectivityManager; + this.listener = listener; + } + + @Override + public boolean register() { + // Initialize isConnected so that we notice the first time around when there's a broadcast. + isConnected = isConnected(); + try { + // See #1405 + context.registerReceiver( + connectivityReceiver, new IntentFilter(ConnectivityManager.CONNECTIVITY_ACTION)); + return true; + } catch (SecurityException e) { + // See #1417, registering the receiver can throw SecurityException. + if (Log.isLoggable(TAG, Log.WARN)) { + Log.w(TAG, "Failed to register", e); + } + return false; + } + } + + @Override + public void unregister() { + context.unregisterReceiver(connectivityReceiver); + } + + @SuppressWarnings("WeakerAccess") + @Synthetic + // Permissions are checked in the factory instead. + @SuppressLint("MissingPermission") + boolean isConnected() { + NetworkInfo networkInfo; + try { + networkInfo = connectivityManager.get().getActiveNetworkInfo(); + } catch (RuntimeException e) { + // #1405 shows that this throws a SecurityException. + // b/70869360 shows that this throws NullPointerException on APIs 22, 23, and 24. + // b/70869360 also shows that this throws RuntimeException on API 24 and 25. + if (Log.isLoggable(TAG, Log.WARN)) { + Log.w(TAG, "Failed to determine connectivity status when connectivity changed", e); + } + // Default to true; + return true; } - // Default to true; - return true; + return networkInfo != null && networkInfo.isConnected(); } - return networkInfo != null && networkInfo.isConnected(); } } diff --git a/library/src/main/java/com/bumptech/glide/util/GlideSuppliers.java b/library/src/main/java/com/bumptech/glide/util/GlideSuppliers.java new file mode 100644 index 0000000000..0003873b4b --- /dev/null +++ b/library/src/main/java/com/bumptech/glide/util/GlideSuppliers.java @@ -0,0 +1,33 @@ +package com.bumptech.glide.util; + +/** Similar to {@link com.google.common.base.Suppliers}, but named to reduce import confusion. */ +public final class GlideSuppliers { + /** + * Produces a non-null instance of {@code T}. + * + * @param The data type + */ + public interface GlideSupplier { + T get(); + } + + private GlideSuppliers() {} + + public static GlideSupplier memorize(final GlideSupplier supplier) { + return new GlideSupplier() { + private volatile T instance; + + @Override + public T get() { + if (instance == null) { + synchronized (this) { + if (instance == null) { + instance = Preconditions.checkNotNull(supplier.get()); + } + } + } + return instance; + } + }; + } +} diff --git a/library/test/src/test/java/com/bumptech/glide/manager/DefaultConnectivityMonitorTest.java b/library/test/src/test/java/com/bumptech/glide/manager/DefaultConnectivityMonitorTest.java index 6acf12445f..b206eebf84 100644 --- a/library/test/src/test/java/com/bumptech/glide/manager/DefaultConnectivityMonitorTest.java +++ b/library/test/src/test/java/com/bumptech/glide/manager/DefaultConnectivityMonitorTest.java @@ -9,14 +9,15 @@ import static org.robolectric.annotation.LooperMode.Mode.LEGACY; import android.app.Application; -import android.content.BroadcastReceiver; import android.content.Context; import android.content.Intent; import android.net.ConnectivityManager; +import android.net.ConnectivityManager.NetworkCallback; +import android.net.Network; import android.net.NetworkInfo; +import android.os.Build; import androidx.test.core.app.ApplicationProvider; import com.bumptech.glide.manager.DefaultConnectivityMonitorTest.PermissionConnectivityManager; -import java.util.List; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -30,11 +31,14 @@ import org.robolectric.annotation.LooperMode; import org.robolectric.shadow.api.Shadow; import org.robolectric.shadows.ShadowConnectivityManager; +import org.robolectric.shadows.ShadowNetwork; import org.robolectric.shadows.ShadowNetworkInfo; @LooperMode(LEGACY) @RunWith(RobolectricTestRunner.class) -@Config(sdk = 18, shadows = PermissionConnectivityManager.class) +@Config( + sdk = {24}, + shadows = PermissionConnectivityManager.class) public class DefaultConnectivityMonitorTest { @Mock private ConnectivityMonitor.ConnectivityListener listener; private DefaultConnectivityMonitor monitor; @@ -44,7 +48,10 @@ public class DefaultConnectivityMonitorTest { public void setUp() { MockitoAnnotations.initMocks(this); monitor = new DefaultConnectivityMonitor(ApplicationProvider.getApplicationContext(), listener); - harness = new ConnectivityHarness(); + harness = + Build.VERSION.SDK_INT >= Build.VERSION_CODES.N + ? new ConnectivityHarnessPost24() + : new ConnectivityHarnessPre24(); } @After @@ -56,7 +63,7 @@ public void tearDown() { public void testRegistersReceiverOnStart() { monitor.onStart(); - assertThat(getConnectivityReceivers()).hasSize(1); + assertThat(harness.getRegisteredReceivers()).isEqualTo(1); } @Test @@ -64,7 +71,7 @@ public void testDoesNotRegisterTwiceOnStart() { monitor.onStart(); monitor.onStart(); - assertThat(getConnectivityReceivers()).hasSize(1); + assertThat(harness.getRegisteredReceivers()).isEqualTo(1); } @Test @@ -72,7 +79,7 @@ public void testUnregistersReceiverOnStop() { monitor.onStart(); monitor.onStop(); - assertThat(getConnectivityReceivers()).isEmpty(); + assertThat(harness.getRegisteredReceivers()).isEqualTo(0); } @Test @@ -80,7 +87,7 @@ public void testHandlesUnregisteringTwiceInARow() { monitor.onStop(); monitor.onStop(); - assertThat(getConnectivityReceivers()).isEmpty(); + assertThat(harness.getRegisteredReceivers()).isEqualTo(0); } @Test @@ -129,7 +136,7 @@ public void testDoesNotNotifyListenerWhenNotRegistered() { @Test public void register_withMissingPermission_doesNotThrow() { - harness.shadowConnectivityManager.isNetworkPermissionGranted = false; + harness.setNetworkPermissionGranted(false); monitor.onStart(); } @@ -137,7 +144,7 @@ public void register_withMissingPermission_doesNotThrow() { @Test public void onReceive_withMissingPermission_doesNotThrow() { monitor.onStart(); - harness.shadowConnectivityManager.isNetworkPermissionGranted = false; + harness.setNetworkPermissionGranted(false); harness.broadcast(); } @@ -145,32 +152,43 @@ public void onReceive_withMissingPermission_doesNotThrow() { public void onReceive_withMissingPermission_previouslyDisconnected_notifiesListenersConnected() { harness.disconnect(); monitor.onStart(); - harness.shadowConnectivityManager.isNetworkPermissionGranted = false; + harness.setNetworkPermissionGranted(false); harness.broadcast(); - verify(listener).onConnectivityChanged(true); + if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N) { + verify(listener).onConnectivityChanged(true); + } else { + verify(listener, never()).onConnectivityChanged(anyBoolean()); + } } @Test public void onReceive_withMissingPermission_previouslyConnected_doesNotNotifyListeners() { harness.connect(); monitor.onStart(); - harness.shadowConnectivityManager.isNetworkPermissionGranted = false; + harness.setNetworkPermissionGranted(false); harness.broadcast(); verify(listener, never()).onConnectivityChanged(anyBoolean()); } - private List getConnectivityReceivers() { - Intent connectivity = new Intent(ConnectivityManager.CONNECTIVITY_ACTION); - return shadowOf((Application) ApplicationProvider.getApplicationContext()) - .getReceiversForIntent(connectivity); + private interface ConnectivityHarness { + void connect(); + + void disconnect(); + + void broadcast(); + + void setNetworkPermissionGranted(boolean isGranted); + + int getRegisteredReceivers(); } - private static class ConnectivityHarness { + private static final class ConnectivityHarnessPost24 implements ConnectivityHarness { + private final PermissionConnectivityManager shadowConnectivityManager; - public ConnectivityHarness() { + ConnectivityHarnessPost24() { ConnectivityManager connectivityManager = (ConnectivityManager) ApplicationProvider.getApplicationContext() @@ -178,25 +196,109 @@ public ConnectivityHarness() { shadowConnectivityManager = Shadow.extract(connectivityManager); } - void disconnect() { + @Override + public void connect() { + shadowConnectivityManager.isConnected = true; + } + + @Override + public void disconnect() { + shadowConnectivityManager.isConnected = false; + } + + @Override + public void broadcast() { + for (NetworkCallback callback : shadowConnectivityManager.getNetworkCallbacks()) { + if (shadowConnectivityManager.isConnected) { + callback.onAvailable(null); + } else { + callback.onLost(null); + } + } + } + + @Override + public void setNetworkPermissionGranted(boolean isGranted) { + shadowConnectivityManager.isNetworkPermissionGranted = isGranted; + } + + @Override + public int getRegisteredReceivers() { + return shadowConnectivityManager.getNetworkCallbacks().size(); + } + } + + private static final class ConnectivityHarnessPre24 implements ConnectivityHarness { + private final PermissionConnectivityManager shadowConnectivityManager; + + public ConnectivityHarnessPre24() { + ConnectivityManager connectivityManager = + (ConnectivityManager) + ApplicationProvider.getApplicationContext() + .getSystemService(Context.CONNECTIVITY_SERVICE); + shadowConnectivityManager = Shadow.extract(connectivityManager); + } + + @Override + public void disconnect() { shadowConnectivityManager.setActiveNetworkInfo(null); } - void connect() { + @Override + public void connect() { NetworkInfo networkInfo = ShadowNetworkInfo.newInstance(NetworkInfo.DetailedState.CONNECTED, 0, 0, true, true); shadowConnectivityManager.setActiveNetworkInfo(networkInfo); } - void broadcast() { + @Override + public void broadcast() { Intent connected = new Intent(ConnectivityManager.CONNECTIVITY_ACTION); ApplicationProvider.getApplicationContext().sendBroadcast(connected); } + + @Override + public void setNetworkPermissionGranted(boolean isGranted) { + shadowConnectivityManager.isNetworkPermissionGranted = isGranted; + } + + @Override + public int getRegisteredReceivers() { + Intent connectivity = new Intent(ConnectivityManager.CONNECTIVITY_ACTION); + return shadowOf((Application) ApplicationProvider.getApplicationContext()) + .getReceiversForIntent(connectivity) + .size(); + } } @Implements(ConnectivityManager.class) public static final class PermissionConnectivityManager extends ShadowConnectivityManager { private boolean isNetworkPermissionGranted = true; + private boolean isConnected; + + @Implementation + @Override + public Network getActiveNetwork() { + if (isConnected) { + return ShadowNetwork.newInstance(1); + } else { + return null; + } + } + + @Implementation + @Override + protected void registerDefaultNetworkCallback(NetworkCallback networkCallback) { + if (!isNetworkPermissionGranted) { + throw new SecurityException(); + } + super.registerDefaultNetworkCallback(networkCallback); + if (isConnected) { + networkCallback.onAvailable(null); + } else { + networkCallback.onLost(null); + } + } @Implementation @Override