Fix bug with deciding when to restart a download.
Steve Howard [Tue, 20 Jul 2010 19:04:02 +0000 (12:04 -0700)]
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

src/com/android/providers/downloads/DownloadInfo.java
src/com/android/providers/downloads/DownloadService.java
tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java

index 5cd50c9..153e045 100644 (file)
@@ -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();
     }
 }
index e474d4d..f870954 100644 (file)
@@ -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);
+        }
     }
 
     /**
index a9810fc..e34c66e 100644 (file)
@@ -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));