Stop image download by canceling a Future. (#3426)

- Switched to stopping an image download by canceling a Future to interrupt the download thread instead of trying to close the InputStream directly since the underlying library does not appear to be thread safe and can throw a variety of Exceptions when close is called on a different thread (#1830 for a previous issue).

Author: gsakakihara

firebase-messaging/src/main/java/com/google/firebase/messaging/DisplayNotification.java

@@ -32,7 +32,7 @@
 import com.google.firebase.messaging.Constants.MessageNotificationKeys;
 import java.util.List;
 import java.util.concurrent.ExecutionException;
-import java.util.concurrent.Executor;
+import java.util.concurrent.ExecutorService;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 
@@ -46,12 +46,12 @@ class DisplayNotification {
 
   private static final int IMAGE_DOWNLOAD_TIMEOUT_SECONDS = 5;
 
-  private final Executor networkIoExecutor;
+  private final ExecutorService networkIoExecutor;
   private final Context context;
   private final NotificationParams params;
 
   public DisplayNotification(
-      Context context, NotificationParams params, Executor networkIoExecutor) {
+      Context context, NotificationParams params, ExecutorService networkIoExecutor) {
     this.networkIoExecutor = networkIoExecutor;
     this.context = context;
     this.params = params;

firebase-messaging/src/main/java/com/google/firebase/messaging/ImageDownload.java

@@ -22,26 +22,30 @@
 import android.util.Log;
 import androidx.annotation.Nullable;
 import com.google.android.gms.tasks.Task;
-import com.google.android.gms.tasks.Tasks;
+import com.google.android.gms.tasks.TaskCompletionSource;
 import com.google.common.io.ByteStreams;
-import com.google.common.io.Closeables;
 import java.io.Closeable;
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.net.URLConnection;
-import java.util.concurrent.Executor;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
 
-/** Abstraction around downloading an image in a background executor. */
-class ImageDownload implements Closeable {
+/**
+ * Abstraction around downloading an image in a background executor.
+ *
+ * @hide
+ */
+public class ImageDownload implements Closeable {
 
   /** Maximum image size to download in bytes (1 MiB). */
   private static final int MAX_IMAGE_SIZE_BYTES = 1024 * 1024;
 
   private final URL url;
+  @Nullable private volatile Future<?> future;
   @Nullable private Task<Bitmap> task;
-  @Nullable private volatile InputStream connectionInputStream;
 
   @Nullable
   public static ImageDownload create(String imageUrl) {
@@ -60,16 +64,29 @@ private ImageDownload(URL url) {
     this.url = url;
   }
 
-  public void start(Executor executor) {
-    task = Tasks.call(executor, this::blockingDownload);
+  public void start(ExecutorService executor) {
+    TaskCompletionSource<Bitmap> taskCompletionSource = new TaskCompletionSource<>();
+    future =
+        executor.submit(
+            () -> {
+              try {
+                Bitmap bitmap = blockingDownload();
+                taskCompletionSource.setResult(bitmap);
+              } catch (Exception e) {
+                taskCompletionSource.setException(e);
+              }
+            });
+    task = taskCompletionSource.getTask();
   }
 
   public Task<Bitmap> getTask() {
     return checkNotNull(task); // will throw if getTask() is called without a call to start()
   }
 
   public Bitmap blockingDownload() throws IOException {
-    Log.i(TAG, "Starting download of: " + url);
+    if (Log.isLoggable(TAG, Log.INFO)) {
+      Log.i(TAG, "Starting download of: " + url);
+    }
 
     byte[] imageBytes = blockingDownloadBytes();
     Bitmap bitmap = BitmapFactory.decodeByteArray(imageBytes, /* offset= */ 0, imageBytes.length);
@@ -96,9 +113,6 @@ private byte[] blockingDownloadBytes() throws IOException {
     // Now actually try to download the content
     byte[] bytes;
     try (InputStream connectionInputStream = connection.getInputStream()) {
-      // Save to a field so that it can be closed on timeout
-      this.connectionInputStream = connectionInputStream;
-
       // Read one byte over the limit so we can tell if the data is too big, as in many cases
       // BitmapFactory will happily decode a partial image.
       bytes =
@@ -118,14 +132,6 @@ private byte[] blockingDownloadBytes() throws IOException {
 
   @Override
   public void close() {
-    // Close the stream to prevent downloaded any additional data. This will cause the input stream
-    // to throw an IOException on read, which will finish the task.
-    try {
-      Closeables.closeQuietly(connectionInputStream);
-    } catch (NullPointerException npe) {
-      // Older versions of okio don't handle closing on a different thread than the one that it was
-      // started on, so just catch it for now.
-      Log.e(TAG, "Failed to close the image download stream.", npe);
-    }
+    future.cancel(true);
   }
 }

firebase-messaging/src/test/java/com/google/firebase/messaging/DisplayNotificationChannelRoboTest.java

@@ -30,7 +30,7 @@
 import androidx.test.core.app.ApplicationProvider;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.concurrent.Executor;
+import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import org.junit.Before;
 import org.junit.Test;
@@ -54,7 +54,7 @@ public final class DisplayNotificationChannelRoboTest {
   Context context = ApplicationProvider.getApplicationContext();
 
   NotificationManager notificationManager;
-  Executor executor;
+  ExecutorService executor;
 
   @Before
   public void setup() throws Exception {
@@ -204,7 +204,7 @@ public void messageProvidedChannel_notExisting() throws Exception {
   }
 
   private static DisplayNotification createDisplayNotification(
-      Context context, Bundle message, Executor executor) {
+      Context context, Bundle message, ExecutorService executor) {
     return new DisplayNotification(context, new NotificationParams(message), executor);
   }
 

firebase-messaging/src/test/java/com/google/firebase/messaging/DisplayNotificationRoboTest.java

@@ -54,7 +54,7 @@
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;
-import java.util.concurrent.Executor;
+import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import org.json.JSONArray;
 import org.junit.Before;
@@ -127,7 +127,7 @@ public class DisplayNotificationRoboTest {
   private ActivityManager activityManager;
   private KeyguardManager keyguardManager;
   private NotificationManager notificationManager;
-  private Executor executor;
+  private ExecutorService executor;
 
   @Before
   public void setUp() throws IOException {

firebase-messaging/src/test/java/com/google/firebase/messaging/ImageDownloadRoboTest.java

@@ -26,7 +26,7 @@
 import java.io.IOException;
 import java.util.Arrays;
 import java.util.concurrent.ExecutionException;
-import java.util.concurrent.Executor;
+import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
 import org.junit.Before;
@@ -46,7 +46,7 @@ public class ImageDownloadRoboTest {
 
   @Rule public TestImageServer testImageServer = new TestImageServer();
 
-  private Executor executor;
+  private ExecutorService executor;
 
   @Before
   public void setUp() throws IOException {