diff options
author | Steve Howard <showard@google.com> | 2010-07-20 12:04:02 -0700 |
---|---|---|
committer | Steve Howard <showard@google.com> | 2010-07-20 12:04:02 -0700 |
commit | 5180de23e77139dd6971dfd48269242e3e3562d9 (patch) | |
tree | ebcf27563c2eb44e821437b459f99f3b150e1454 | |
parent | 071bd7acb3185f4f1e807855605c5e6018e9742f (diff) | |
download | android_packages_providers_DownloadProvider-5180de23e77139dd6971dfd48269242e3e3562d9.tar.gz android_packages_providers_DownloadProvider-5180de23e77139dd6971dfd48269242e3e3562d9.tar.bz2 android_packages_providers_DownloadProvider-5180de23e77139dd6971dfd48269242e3e3562d9.zip |
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
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)); |