Cleaner thread management, less global state.

Switch to using a ThreadPoolExecutor for handling downloads, which
gives us parallelism logic that is easier to reason about.  Also
open the door to eventually waiting until the executor is drained
to stopSelf().

Removes DownloadHandler singleton, and gives explicit path for
publishing active download speeds to notifications.

Change-Id: I1836e7742bb8a84861d1ca6bd1e59b2040bd12f8

5 files changed, 101 insertions, 149 deletions

diff --git a/src/com/android/providers/downloads/DownloadHandler.java b/src/com/android/providers/downloads/DownloadHandler.java
deleted file mode 100644
index c376ff1e..00000000
--- a/src/com/android/providers/downloads/DownloadHandler.java
+++ /dev/null
@@ -1,99 +0,0 @@
-/*
- * Copyright (C) 2011 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *      http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package com.android.providers.downloads;
-
-import android.content.res.Resources;
-import android.util.Log;
-import android.util.LongSparseArray;
-
-import com.android.internal.annotations.GuardedBy;
-
-import java.util.ArrayList;
-import java.util.HashMap;
-import java.util.Iterator;
-import java.util.LinkedHashMap;
-
-public class DownloadHandler {
-    private static final String TAG = "DownloadHandler";
-
-    @GuardedBy("this")
-    private final LinkedHashMap<Long, DownloadInfo> mDownloadsQueue =
-            new LinkedHashMap<Long, DownloadInfo>();
-    @GuardedBy("this")
-    private final HashMap<Long, DownloadInfo> mDownloadsInProgress =
-            new HashMap<Long, DownloadInfo>();
-    @GuardedBy("this")
-    private final LongSparseArray<Long> mCurrentSpeed = new LongSparseArray<Long>();
-
-    private final int mMaxConcurrentDownloadsAllowed = Resources.getSystem().getInteger(
-            com.android.internal.R.integer.config_MaxConcurrentDownloadsAllowed);
-
-    private static final DownloadHandler sDownloadHandler = new DownloadHandler();
-
-    public static DownloadHandler getInstance() {
-        return sDownloadHandler;
-    }
-
-    public synchronized void enqueueDownload(DownloadInfo info) {
-        if (!mDownloadsQueue.containsKey(info.mId)) {
-            if (Constants.LOGV) {
-                Log.i(TAG, "enqueued download. id: " + info.mId + ", uri: " + info.mUri);
-            }
-            mDownloadsQueue.put(info.mId, info);
-            startDownloadThreadLocked();
-        }
-    }
-
-    private void startDownloadThreadLocked() {
-        Iterator<Long> keys = mDownloadsQueue.keySet().iterator();
-        ArrayList<Long> ids = new ArrayList<Long>();
-        while (mDownloadsInProgress.size() < mMaxConcurrentDownloadsAllowed && keys.hasNext()) {
-            Long id = keys.next();
-            DownloadInfo info = mDownloadsQueue.get(id);
-            info.startDownloadThread();
-            ids.add(id);
-            mDownloadsInProgress.put(id, mDownloadsQueue.get(id));
-            if (Constants.LOGV) {
-                Log.i(TAG, "started download for : " + id);
-            }
-        }
-        for (Long id : ids) {
-            mDownloadsQueue.remove(id);
-        }
-    }
-
-    public synchronized boolean hasDownloadInQueue(long id) {
-        return mDownloadsQueue.containsKey(id) || mDownloadsInProgress.containsKey(id);
-    }
-
-    public synchronized void dequeueDownload(long id) {
-        mDownloadsInProgress.remove(id);
-        mCurrentSpeed.remove(id);
-        startDownloadThreadLocked();
-        if (mDownloadsInProgress.size() == 0 && mDownloadsQueue.size() == 0) {
-            notifyAll();
-        }
-    }
-
-    public synchronized void setCurrentSpeed(long id, long speed) {
-        mCurrentSpeed.put(id, speed);
-    }
-
-    public synchronized long getCurrentSpeed(long id) {
-        return mCurrentSpeed.get(id, -1L);
-    }
-}
diff --git a/src/com/android/providers/downloads/DownloadInfo.java b/src/com/android/providers/downloads/DownloadInfo.java
index b984bdef..74b52d48 100644
--- a/src/com/android/providers/downloads/DownloadInfo.java
+++ b/src/com/android/providers/downloads/DownloadInfo.java
@@ -31,15 +31,18 @@ import android.os.Environment;
 import android.provider.Downloads;
 import android.provider.Downloads.Impl;
 import android.text.TextUtils;
-import android.util.Log;
 import android.util.Pair;
 
+import com.android.internal.annotations.GuardedBy;
 import com.android.internal.util.IndentingPrintWriter;
 
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.concurrent.Executor;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Future;
 
 /**
  * Stores information about an individual download.
@@ -58,8 +61,9 @@ public class DownloadInfo {
         }
 
         public DownloadInfo newDownloadInfo(Context context, SystemFacade systemFacade,
-                StorageManager storageManager) {
-            DownloadInfo info = new DownloadInfo(context, systemFacade, storageManager);
+                StorageManager storageManager, DownloadNotifier notifier) {
+            final DownloadInfo info = new DownloadInfo(
+                    context, systemFacade, storageManager, notifier);
             updateFromDatabase(info);
             readRequestHeaders(info);
             return info;
@@ -200,7 +204,6 @@ public class DownloadInfo {
      */
     public static final String EXTRA_IS_WIFI_REQUIRED = "isWifiRequired";
 
-
     public long mId;
     public String mUri;
     public boolean mNoIntegrity;
@@ -239,14 +242,24 @@ public class DownloadInfo {
 
     private List<Pair<String, String>> mRequestHeaders = new ArrayList<Pair<String, String>>();
 
+    /**
+     * Result of last {@link DownloadThread} started by
+     * {@link #startIfReady(ExecutorService)}.
+     */
+    @GuardedBy("this")
+    private Future<?> mActiveTask;
+
     private final Context mContext;
     private final SystemFacade mSystemFacade;
     private final StorageManager mStorageManager;
+    private final DownloadNotifier mNotifier;
 
-    private DownloadInfo(Context context, SystemFacade systemFacade, StorageManager storageManager) {
+    private DownloadInfo(Context context, SystemFacade systemFacade, StorageManager storageManager,
+            DownloadNotifier notifier) {
         mContext = context;
         mSystemFacade = systemFacade;
         mStorageManager = storageManager;
+        mNotifier = notifier;
         mFuzz = Helpers.sRandom.nextInt(1001);
     }
 
@@ -297,14 +310,9 @@ public class DownloadInfo {
     }
 
     /**
-     * Returns whether this download (which the download manager hasn't seen yet)
-     * should be started.
+     * Returns whether this download should be enqueued.
      */
-    private boolean isReadyToStart(long now) {
-        if (DownloadHandler.getInstance().hasDownloadInQueue(mId)) {
-            // already running
-            return false;
-        }
+    private boolean isReadyToStart() {
         if (mControl == Downloads.Impl.CONTROL_PAUSED) {
             // the download is paused, so it's not going to start
             return false;
@@ -322,6 +330,7 @@ public class DownloadInfo {
 
             case Downloads.Impl.STATUS_WAITING_TO_RETRY:
                 // download was waiting for a delayed restart
+                final long now = mSystemFacade.currentTimeMillis();
                 return restartTime(now) <= now;
             case Downloads.Impl.STATUS_DEVICE_NOT_FOUND_ERROR:
                 // is the media mounted?
@@ -437,21 +446,27 @@ public class DownloadInfo {
         return NetworkState.OK;
     }
 
-    void startIfReady(long now, StorageManager storageManager) {
-        if (!isReadyToStart(now)) {
-            return;
-        }
+    /**
+     * If download is ready to start, and isn't already pending or executing,
+     * create a {@link DownloadThread} and enqueue it into given
+     * {@link Executor}.
+     */
+    public void startIfReady(ExecutorService executor) {
+        synchronized (this) {
+            final boolean isActive = mActiveTask != null && !mActiveTask.isDone();
+            if (isReadyToStart() && !isActive) {
+                if (mStatus != Impl.STATUS_RUNNING) {
+                    mStatus = Impl.STATUS_RUNNING;
+                    ContentValues values = new ContentValues();
+                    values.put(Impl.COLUMN_STATUS, mStatus);
+                    mContext.getContentResolver().update(getAllDownloadsUri(), values, null, null);
+                }
 
-        if (Constants.LOGV) {
-            Log.v(Constants.TAG, "Service spawning thread to handle download " + mId);
-        }
-        if (mStatus != Impl.STATUS_RUNNING) {
-            mStatus = Impl.STATUS_RUNNING;
-            ContentValues values = new ContentValues();
-            values.put(Impl.COLUMN_STATUS, mStatus);
-            mContext.getContentResolver().update(getAllDownloadsUri(), values, null, null);
+                final DownloadThread task = new DownloadThread(
+                        mContext, mSystemFacade, this, mStorageManager, mNotifier);
+                mActiveTask = executor.submit(task);
+            }
         }
-        DownloadHandler.getInstance().enqueueDownload(this);
     }
 
     public boolean isOnCache() {
@@ -553,11 +568,6 @@ public class DownloadInfo {
         mContext.startActivity(intent);
     }
 
-    void startDownloadThread() {
-        // TODO: keep this thread strongly referenced
-        new DownloadThread(mContext, mSystemFacade, this, mStorageManager).start();
-    }
-
     /**
      * Query and return status of requested download.
      */
diff --git a/src/com/android/providers/downloads/DownloadNotifier.java b/src/com/android/providers/downloads/DownloadNotifier.java
index daae7831..2dd9805a 100644
--- a/src/com/android/providers/downloads/DownloadNotifier.java
+++ b/src/com/android/providers/downloads/DownloadNotifier.java
@@ -32,6 +32,7 @@ import android.net.Uri;
 import android.provider.Downloads;
 import android.text.TextUtils;
 import android.text.format.DateUtils;
+import android.util.LongSparseLongArray;
 
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.Maps;
@@ -66,6 +67,13 @@ public class DownloadNotifier {
     @GuardedBy("mActiveNotifs")
     private final HashMap<String, Long> mActiveNotifs = Maps.newHashMap();
 
+    /**
+     * Current speed of active downloads, mapped from {@link DownloadInfo#mId}
+     * to speed in bytes per second.
+     */
+    @GuardedBy("mDownloadSpeed")
+    private final LongSparseLongArray mDownloadSpeed = new LongSparseLongArray();
+
     public DownloadNotifier(Context context) {
         mContext = context;
         mNotifManager = (NotificationManager) context.getSystemService(
@@ -77,6 +85,20 @@ public class DownloadNotifier {
     }
 
     /**
+     * Notify the current speed of an active download, used for calcuating
+     * estimated remaining time.
+     */
+    public void notifyDownloadSpeed(long id, long bytesPerSecond) {
+        synchronized (mDownloadSpeed) {
+            if (bytesPerSecond != 0) {
+                mDownloadSpeed.put(id, bytesPerSecond);
+            } else {
+                mDownloadSpeed.delete(id);
+            }
+        }
+    }
+
+    /**
      * Update {@link NotificationManager} to reflect the given set of
      * {@link DownloadInfo}, adding, collapsing, and removing as needed.
      */
@@ -163,16 +185,16 @@ public class DownloadNotifier {
             String remainingText = null;
             String percentText = null;
             if (type == TYPE_ACTIVE) {
-                final DownloadHandler handler = DownloadHandler.getInstance();
-
                 long current = 0;
                 long total = 0;
                 long speed = 0;
-                for (DownloadInfo info : cluster) {
-                    if (info.mTotalBytes != -1) {
-                        current += info.mCurrentBytes;
-                        total += info.mTotalBytes;
-                        speed += handler.getCurrentSpeed(info.mId);
+                synchronized (mDownloadSpeed) {
+                    for (DownloadInfo info : cluster) {
+                        if (info.mTotalBytes != -1) {
+                            current += info.mCurrentBytes;
+                            total += info.mTotalBytes;
+                            speed += mDownloadSpeed.get(info.mId);
+                        }
                     }
                 }
 
diff --git a/src/com/android/providers/downloads/DownloadService.java b/src/com/android/providers/downloads/DownloadService.java
index 4a1b40d5..039f12cd 100644
--- a/src/com/android/providers/downloads/DownloadService.java
+++ b/src/com/android/providers/downloads/DownloadService.java
@@ -26,6 +26,7 @@ import android.content.ContentValues;
 import android.content.Context;
 import android.content.Intent;
 import android.content.ServiceConnection;
+import android.content.res.Resources;
 import android.database.ContentObserver;
 import android.database.Cursor;
 import android.media.IMediaScannerListener;
@@ -53,6 +54,10 @@ import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadPoolExecutor;
+import java.util.concurrent.TimeUnit;
 
 /**
  * Performs the background downloads requested by applications that use the Downloads provider.
@@ -76,12 +81,26 @@ public class DownloadService extends Service {
     @GuardedBy("mDownloads")
     private Map<Long, DownloadInfo> mDownloads = Maps.newHashMap();
 
+    private final ExecutorService mExecutor = buildDownloadExecutor();
+
+    private static ExecutorService buildDownloadExecutor() {
+        final int maxConcurrent = Resources.getSystem().getInteger(
+                com.android.internal.R.integer.config_MaxConcurrentDownloadsAllowed);
+
+        // Create a bounded thread pool for executing downloads; it creates
+        // threads as needed (up to maximum) and reclaims them when finished.
+        final ThreadPoolExecutor executor = new ThreadPoolExecutor(
+                maxConcurrent, maxConcurrent, 10, TimeUnit.SECONDS,
+                new LinkedBlockingQueue<Runnable>());
+        executor.allowCoreThreadTimeOut(true);
+        return executor;
+    }
+
     /**
      * The thread that updates the internal download list from the content
      * provider.
      */
-    @VisibleForTesting
-    UpdateThread mUpdateThread;
+    private UpdateThread mUpdateThread;
 
     /**
      * Whether the internal download list should be updated from the content
@@ -435,14 +454,15 @@ public class DownloadService extends Service {
      * download if appropriate.
      */
     private DownloadInfo insertDownloadLocked(DownloadInfo.Reader reader, long now) {
-        DownloadInfo info = reader.newDownloadInfo(this, mSystemFacade, mStorageManager);
+        final DownloadInfo info = reader.newDownloadInfo(
+                this, mSystemFacade, mStorageManager, mNotifier);
         mDownloads.put(info.mId, info);
 
         if (Constants.LOGVV) {
             Log.v(Constants.TAG, "processing inserted download " + info.mId);
         }
 
-        info.startIfReady(now, mStorageManager);
+        info.startIfReady(mExecutor);
         return info;
     }
 
@@ -458,7 +478,7 @@ public class DownloadService extends Service {
             Log.v(Constants.TAG, "processing updated download " + info.mId +
                     ", status: " + info.mStatus);
         }
-        info.startIfReady(now, mStorageManager);
+        info.startIfReady(mExecutor);
     }
 
     /**
diff --git a/src/com/android/providers/downloads/DownloadThread.java b/src/com/android/providers/downloads/DownloadThread.java
index 5bb1e9bd..bd347e42 100644
--- a/src/com/android/providers/downloads/DownloadThread.java
+++ b/src/com/android/providers/downloads/DownloadThread.java
@@ -68,10 +68,10 @@ import libcore.io.IoUtils;
 import libcore.net.http.HttpEngine;
 
 /**
- * Thread which executes a given {@link DownloadInfo}: making network requests,
+ * Task which executes a given {@link DownloadInfo}: making network requests,
  * persisting data to disk, and updating {@link DownloadProvider}.
  */
-public class DownloadThread extends Thread {
+public class DownloadThread implements Runnable {
 
     private static final int HTTP_REQUESTED_RANGE_NOT_SATISFIABLE = 416;
     private static final int HTTP_TEMP_REDIRECT = 307;
@@ -82,15 +82,17 @@ public class DownloadThread extends Thread {
     private final DownloadInfo mInfo;
     private final SystemFacade mSystemFacade;
     private final StorageManager mStorageManager;
+    private final DownloadNotifier mNotifier;
 
     private volatile boolean mPolicyDirty;
 
     public DownloadThread(Context context, SystemFacade systemFacade, DownloadInfo info,
-            StorageManager storageManager) {
+            StorageManager storageManager, DownloadNotifier notifier) {
         mContext = context;
         mSystemFacade = systemFacade;
         mInfo = info;
         mStorageManager = storageManager;
+        mNotifier = notifier;
     }
 
     /**
@@ -151,16 +153,13 @@ public class DownloadThread extends Thread {
         }
     }
 
-    /**
-     * Executes the download in a separate thread
-     */
     @Override
     public void run() {
         Process.setThreadPriority(Process.THREAD_PRIORITY_BACKGROUND);
         try {
             runInternal();
         } finally {
-            DownloadHandler.getInstance().dequeueDownload(mInfo.mId);
+            mNotifier.notifyDownloadSpeed(mInfo.mId, 0);
         }
     }
 
@@ -526,7 +525,7 @@ public class DownloadThread extends Thread {
             state.mSpeedSampleStart = now;
             state.mSpeedSampleBytes = state.mCurrentBytes;
 
-            DownloadHandler.getInstance().setCurrentSpeed(mInfo.mId, state.mSpeed);
+            mNotifier.notifyDownloadSpeed(mInfo.mId, state.mSpeed);
         }
 
         if (state.mCurrentBytes - state.mBytesNotified > Constants.MIN_PROGRESS_STEP &&