From 5180de23e77139dd6971dfd48269242e3e3562d9 Mon Sep 17 00:00:00 2001
From: Steve Howard <showard@google.com>
Date: Tue, 20 Jul 2010 12:04:02 -0700
Subject: Fix bug with deciding when to restart a download.

In previous refactoring I had combined the code for starting a
download (from DownloadService.insertDownload()) and restarting a
download (from DownloadService.updateDownload()).  I'd missed the fact
that the former checks DownloadInfo.isReadyToStart() while the latter
checks DownloadInfo.isReadyToRestart().  This cause a race condition
where multiple startService() calls during a download would cause the
service to try to spawn a second thread for the same running download.

I've fixed the bug and added a test case to exercise this.

Change-Id: Ia968331a030137daac7c8b5792a01e3e19065b38
---
 .../android/providers/downloads/DownloadInfo.java  | 36 ++++++++++------------
 .../providers/downloads/DownloadService.java       |  8 +++--
 .../downloads/PublicApiFunctionalTest.java         | 13 ++++++++
 3 files changed, 36 insertions(+), 21 deletions(-)

diff --git a/src/com/android/providers/downloads/DownloadInfo.java b/src/com/android/providers/downloads/DownloadInfo.java
index 5cd50c92..153e0454 100644
--- a/src/com/android/providers/downloads/DownloadInfo.java
+++ b/src/com/android/providers/downloads/DownloadInfo.java
@@ -289,25 +289,23 @@ public class DownloadInfo {
         return mTotalBytes <= maxBytesOverMobile;
     }
 
-    void startIfReady(long now) {
-        if (isReadyToStart(now)) {
-            if (Constants.LOGV) {
-                Log.v(Constants.TAG, "Service spawning thread to handle download " + mId);
-            }
-            if (mHasActiveThread) {
-                throw new IllegalStateException("Multiple threads on same download");
-            }
-            if (mStatus != Impl.STATUS_RUNNING) {
-                mStatus = Impl.STATUS_RUNNING;
-                ContentValues values = new ContentValues();
-                values.put(Impl.COLUMN_STATUS, mStatus);
-                mContext.getContentResolver().update(
-                        ContentUris.withAppendedId(Impl.CONTENT_URI, mId),
-                        values, null, null);
-            }
-            DownloadThread downloader = new DownloadThread(mContext, mSystemFacade, this);
-            mHasActiveThread = true;
-            downloader.start();
+    void start(long now) {
+        if (Constants.LOGV) {
+            Log.v(Constants.TAG, "Service spawning thread to handle download " + mId);
+        }
+        if (mHasActiveThread) {
+            throw new IllegalStateException("Multiple threads on same download");
+        }
+        if (mStatus != Impl.STATUS_RUNNING) {
+            mStatus = Impl.STATUS_RUNNING;
+            ContentValues values = new ContentValues();
+            values.put(Impl.COLUMN_STATUS, mStatus);
+            mContext.getContentResolver().update(
+                    ContentUris.withAppendedId(Impl.CONTENT_URI, mId),
+                    values, null, null);
         }
+        DownloadThread downloader = new DownloadThread(mContext, mSystemFacade, this);
+        mHasActiveThread = true;
+        downloader.start();
     }
 }
diff --git a/src/com/android/providers/downloads/DownloadService.java b/src/com/android/providers/downloads/DownloadService.java
index e474d4d7..f870954b 100644
--- a/src/com/android/providers/downloads/DownloadService.java
+++ b/src/com/android/providers/downloads/DownloadService.java
@@ -614,7 +614,9 @@ public class DownloadService extends Service {
             }
         }
 
-        info.startIfReady(now);
+        if (info.isReadyToStart(now)) {
+            info.start(now);
+        }
     }
 
     /**
@@ -674,7 +676,9 @@ public class DownloadService extends Service {
         info.mMediaScanned =
                 cursor.getInt(cursor.getColumnIndexOrThrow(Constants.MEDIA_SCANNED)) == 1;
 
-        info.startIfReady(now);
+        if (info.isReadyToRestart(now)) {
+            info.start(now);
+        }
     }
 
     /**
diff --git a/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java b/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java
index a9810fc1..e34c66e6 100644
--- a/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java
+++ b/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java
@@ -309,6 +309,19 @@ public class PublicApiFunctionalTest extends AbstractDownloadManagerFunctionalTe
         enqueueResponse(HTTP_OK, FILE_CONTENT);
         download.runUntilStatus(DownloadManager.STATUS_SUCCESSFUL);
     }
+    
+    /**
+     * Test for race conditions when the service is flooded with startService() calls while running
+     * a download.
+     */
+    public void testFloodServiceWithStarts() throws Exception {
+        enqueueResponse(HTTP_OK, FILE_CONTENT);
+        Download download = enqueueRequest(getRequest());
+        while (download.getStatus() != DownloadManager.STATUS_SUCCESSFUL) {
+            startService(null);
+            Thread.sleep(10);
+        }
+    }
 
     private DownloadManager.Request getRequest() throws MalformedURLException {
         return getRequest(getServerUri(REQUEST_PATH));
-- 
cgit v1.2.3