From 3a5f5eafb34eaa4963c801882148e8f61514a61b Mon Sep 17 00:00:00 2001 From: Jeff Sharkey Date: Wed, 20 Apr 2016 23:23:09 -0600 Subject: Move DownloadManager to use JobScheduler. JobScheduler is in a much better position to coordinate tasks across the platform to optimize battery and RAM usage. This change removes a bunch of manual scheduling logic by representing each download as a separate job with relevant scheduling constraints. Requested network types, retry backoff timing, and newly added charging and idle constraints are plumbed through as job parameters. When a job times out, we halt the download and schedule it to resume later. The majority of downloads should have ETag values to enable resuming like this. Remove local wakelocks, since the platform now acquires and blames our jobs on the requesting app. When an active download is pushing updates to the database, check for both paused and cancelled state to quickly halt an ongoing download. Shift DownloadNotifier to update directly based on a Cursor, since we no longer have the overhead of fully-parsed DownloadInfo objects. Unify a handful of worker threads into a single shared thread. Remove legacy "large download" activity that was thrown in the face of the user; the UX best-practice is to go through notification, and update that dialog to let the user override and continue if under the hard limit. Bug: 28098882, 26571724 Change-Id: I33ebe59b3c2ea9c89ec526f70b1950c734abc4a7 --- .../AbstractDownloadProviderFunctionalTest.java | 35 +++++++------- .../providers/downloads/AbstractPublicApiTest.java | 7 +-- .../downloads/DownloadProviderFunctionalTest.java | 9 ++-- .../providers/downloads/FakeSystemFacade.java | 55 ++++++++++++++-------- .../downloads/PublicApiFunctionalTest.java | 42 +++++++++-------- .../android/providers/downloads/ThreadingTest.java | 17 +------ 6 files changed, 83 insertions(+), 82 deletions(-) (limited to 'tests/src') diff --git a/tests/src/com/android/providers/downloads/AbstractDownloadProviderFunctionalTest.java b/tests/src/com/android/providers/downloads/AbstractDownloadProviderFunctionalTest.java index 6934b86d..0330fd38 100644 --- a/tests/src/com/android/providers/downloads/AbstractDownloadProviderFunctionalTest.java +++ b/tests/src/com/android/providers/downloads/AbstractDownloadProviderFunctionalTest.java @@ -17,13 +17,14 @@ package com.android.providers.downloads; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import android.app.DownloadManager; import android.app.NotificationManager; -import android.content.ComponentName; +import android.app.job.JobParameters; +import android.app.job.JobScheduler; import android.content.ContentResolver; import android.content.Context; -import android.content.Intent; import android.content.pm.ProviderInfo; import android.database.ContentObserver; import android.database.Cursor; @@ -49,7 +50,7 @@ import java.net.MalformedURLException; import java.net.UnknownHostException; public abstract class AbstractDownloadProviderFunctionalTest extends - ServiceTestCase { + ServiceTestCase { protected static final String LOG_TAG = "DownloadProviderFunctionalTest"; private static final String PROVIDER_AUTHORITY = "downloads"; @@ -102,14 +103,14 @@ public abstract class AbstractDownloadProviderFunctionalTest extends private final ContentResolver mResolver; private final NotificationManager mNotifManager; private final DownloadManager mDownloadManager; - - boolean mHasServiceBeenStarted = false; + private final JobScheduler mJobScheduler; public TestContext(Context realContext) { super(realContext, FILENAME_PREFIX); mResolver = new MockContentResolverWithNotify(this); mNotifManager = mock(NotificationManager.class); mDownloadManager = mock(DownloadManager.class); + mJobScheduler = mock(JobScheduler.class); } /** @@ -129,26 +130,16 @@ public abstract class AbstractDownloadProviderFunctionalTest extends return mNotifManager; } else if (Context.DOWNLOAD_SERVICE.equals(name)) { return mDownloadManager; + } else if (Context.JOB_SCHEDULER_SERVICE.equals(name)) { + return mJobScheduler; } return super.getSystemService(name); } - - /** - * Record when DownloadProvider starts DownloadService. - */ - @Override - public ComponentName startService(Intent service) { - if (service.getComponent().getClassName().equals(DownloadService.class.getName())) { - mHasServiceBeenStarted = true; - return service.getComponent(); - } - throw new UnsupportedOperationException("Unexpected service: " + service); - } } public AbstractDownloadProviderFunctionalTest(FakeSystemFacade systemFacade) { - super(DownloadService.class); + super(DownloadJobService.class); mSystemFacade = systemFacade; } @@ -177,7 +168,7 @@ public abstract class AbstractDownloadProviderFunctionalTest extends setContext(mTestContext); setupService(); - getService().mSystemFacade = mSystemFacade; + Helpers.setSystemFacade(mSystemFacade); mSystemFacade.setUp(); assertTrue(isDatabaseEmpty()); // ensure we're not messing with real data @@ -193,6 +184,12 @@ public abstract class AbstractDownloadProviderFunctionalTest extends super.tearDown(); } + protected void startDownload(long id) { + final JobParameters params = mock(JobParameters.class); + when(params.getJobId()).thenReturn((int) id); + getService().onStartJob(params); + } + private boolean isDatabaseEmpty() { Cursor cursor = mResolver.query(Downloads.Impl.ALL_DOWNLOADS_CONTENT_URI, null, null, null, null); diff --git a/tests/src/com/android/providers/downloads/AbstractPublicApiTest.java b/tests/src/com/android/providers/downloads/AbstractPublicApiTest.java index c0a1108b..3a585b47 100644 --- a/tests/src/com/android/providers/downloads/AbstractPublicApiTest.java +++ b/tests/src/com/android/providers/downloads/AbstractPublicApiTest.java @@ -115,13 +115,13 @@ public abstract class AbstractPublicApiTest extends AbstractDownloadProviderFunc void runUntilStatus(int status) throws TimeoutException { final long startMillis = mSystemFacade.currentTimeMillis(); - startService(null); + startDownload(mId); waitForStatus(status, startMillis); } void runUntilStatus(int status, long timeout) throws TimeoutException { final long startMillis = mSystemFacade.currentTimeMillis(); - startService(null); + startDownload(mId); waitForStatus(status, startMillis, timeout); } @@ -169,7 +169,7 @@ public abstract class AbstractPublicApiTest extends AbstractDownloadProviderFunc // waits until progress_so_far is >= (progress)% boolean runUntilProgress(int progress) throws InterruptedException { - startService(null); + startDownload(mId); int sleepCounter = MAX_TIME_TO_WAIT_FOR_OPERATION * 1000 / TIME_TO_SLEEP; int numBytesReceivedSoFar = 0; @@ -230,6 +230,7 @@ public abstract class AbstractPublicApiTest extends AbstractDownloadProviderFunc return PACKAGE_NAME; } }); + mManager.setAccessFilename(true); } protected DownloadManager.Request getRequest() diff --git a/tests/src/com/android/providers/downloads/DownloadProviderFunctionalTest.java b/tests/src/com/android/providers/downloads/DownloadProviderFunctionalTest.java index 3b651048..9a4e6444 100644 --- a/tests/src/com/android/providers/downloads/DownloadProviderFunctionalTest.java +++ b/tests/src/com/android/providers/downloads/DownloadProviderFunctionalTest.java @@ -17,8 +17,10 @@ package com.android.providers.downloads; import static android.text.format.DateUtils.SECOND_IN_MILLIS; + import static java.net.HttpURLConnection.HTTP_OK; +import android.content.ContentUris; import android.content.ContentValues; import android.database.Cursor; import android.net.ConnectivityManager; @@ -56,7 +58,6 @@ public class DownloadProviderFunctionalTest extends AbstractDownloadProviderFunc String path = "/download_manager_test_path"; Uri downloadUri = requestDownload(path); assertEquals(Downloads.Impl.STATUS_PENDING, getDownloadStatus(downloadUri)); - assertTrue(mTestContext.mHasServiceBeenStarted); runUntilStatus(downloadUri, Downloads.Impl.STATUS_SUCCESS); RecordedRequest request = takeRequest(); @@ -108,13 +109,11 @@ public class DownloadProviderFunctionalTest extends AbstractDownloadProviderFunc // Assert that HTTP request succeeds when cleartext traffic is permitted mSystemFacade.mCleartextTrafficPermitted = true; Uri downloadUri = requestDownload("/path"); - assertEquals("http", downloadUri.getScheme()); runUntilStatus(downloadUri, Downloads.Impl.STATUS_SUCCESS); // Assert that HTTP request fails when cleartext traffic is not permitted mSystemFacade.mCleartextTrafficPermitted = false; downloadUri = requestDownload("/path"); - assertEquals("http", downloadUri.getScheme()); runUntilStatus(downloadUri, Downloads.Impl.STATUS_BAD_REQUEST); } @@ -131,8 +130,8 @@ public class DownloadProviderFunctionalTest extends AbstractDownloadProviderFunc } private void runUntilStatus(Uri downloadUri, int expected) throws Exception { - startService(null); - + startDownload(ContentUris.parseId(downloadUri)); + int actual = -1; final long timeout = SystemClock.elapsedRealtime() + (15 * SECOND_IN_MILLIS); diff --git a/tests/src/com/android/providers/downloads/FakeSystemFacade.java b/tests/src/com/android/providers/downloads/FakeSystemFacade.java index af5482e1..eaf5e43d 100644 --- a/tests/src/com/android/providers/downloads/FakeSystemFacade.java +++ b/tests/src/com/android/providers/downloads/FakeSystemFacade.java @@ -1,5 +1,9 @@ package com.android.providers.downloads; +import static org.mockito.Matchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + import android.content.Intent; import android.content.pm.PackageManager.NameNotFoundException; import android.net.ConnectivityManager; @@ -7,16 +11,22 @@ import android.net.Network; import android.net.NetworkInfo; import android.net.NetworkInfo.DetailedState; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; + +import java.io.IOException; +import java.net.URL; +import java.net.URLConnection; import java.util.ArrayList; import java.util.List; + public class FakeSystemFacade implements SystemFacade { long mTimeMillis = 0; - Network mActiveNetwork = null; Integer mActiveNetworkType = ConnectivityManager.TYPE_WIFI; boolean mIsRoaming = false; boolean mIsMetered = false; - Long mMaxBytesOverMobile = null; - Long mRecommendedMaxBytesOverMobile = null; + long mMaxBytesOverMobile = Long.MAX_VALUE; + long mRecommendedMaxBytesOverMobile = Long.MAX_VALUE; List mBroadcastsSent = new ArrayList(); boolean mCleartextTrafficPermitted = true; private boolean mReturnActualTime = false; @@ -26,8 +36,8 @@ public class FakeSystemFacade implements SystemFacade { mActiveNetworkType = ConnectivityManager.TYPE_WIFI; mIsRoaming = false; mIsMetered = false; - mMaxBytesOverMobile = null; - mRecommendedMaxBytesOverMobile = null; + mMaxBytesOverMobile = Long.MAX_VALUE; + mRecommendedMaxBytesOverMobile = Long.MAX_VALUE; mBroadcastsSent.clear(); mReturnActualTime = false; } @@ -46,37 +56,44 @@ public class FakeSystemFacade implements SystemFacade { @Override public Network getActiveNetwork(int uid) { - return mActiveNetwork; + if (mActiveNetworkType == null) { + return null; + } else { + final Network network = mock(Network.class); + try { + when(network.openConnection(any())).then(new Answer() { + @Override + public URLConnection answer(InvocationOnMock invocation) throws Throwable { + final URL url = (URL) invocation.getArguments()[0]; + return url.openConnection(); + } + }); + } catch (IOException ignored) { + } + return network; + } } @Override - public NetworkInfo getActiveNetworkInfo(int uid) { + public NetworkInfo getNetworkInfo(Network network) { if (mActiveNetworkType == null) { return null; } else { final NetworkInfo info = new NetworkInfo(mActiveNetworkType, 0, null, null); info.setDetailedState(DetailedState.CONNECTED, null, null); + info.setRoaming(mIsRoaming); + info.setMetered(mIsMetered); return info; } } @Override - public boolean isActiveNetworkMetered() { - return mIsMetered; - } - - @Override - public boolean isNetworkRoaming() { - return mIsRoaming; - } - - @Override - public Long getMaxBytesOverMobile() { + public long getMaxBytesOverMobile() { return mMaxBytesOverMobile; } @Override - public Long getRecommendedMaxBytesOverMobile() { + public long getRecommendedMaxBytesOverMobile() { return mRecommendedMaxBytesOverMobile; } diff --git a/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java b/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java index 17fed6d0..97bc4a22 100644 --- a/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java +++ b/tests/src/com/android/providers/downloads/PublicApiFunctionalTest.java @@ -20,12 +20,7 @@ import static android.app.DownloadManager.STATUS_FAILED; import static android.app.DownloadManager.STATUS_PAUSED; import static android.net.TrafficStats.GB_IN_BYTES; import static android.text.format.DateUtils.SECOND_IN_MILLIS; -import static java.net.HttpURLConnection.HTTP_MOVED_TEMP; -import static java.net.HttpURLConnection.HTTP_NOT_FOUND; -import static java.net.HttpURLConnection.HTTP_OK; -import static java.net.HttpURLConnection.HTTP_PARTIAL; -import static java.net.HttpURLConnection.HTTP_PRECON_FAILED; -import static java.net.HttpURLConnection.HTTP_UNAVAILABLE; + import static org.mockito.Matchers.anyInt; import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.isA; @@ -34,10 +29,16 @@ import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static java.net.HttpURLConnection.HTTP_MOVED_TEMP; +import static java.net.HttpURLConnection.HTTP_NOT_FOUND; +import static java.net.HttpURLConnection.HTTP_OK; +import static java.net.HttpURLConnection.HTTP_PARTIAL; +import static java.net.HttpURLConnection.HTTP_PRECON_FAILED; +import static java.net.HttpURLConnection.HTTP_UNAVAILABLE; + import android.app.DownloadManager; import android.app.Notification; import android.app.NotificationManager; -import android.content.Context; import android.content.Intent; import android.database.Cursor; import android.net.ConnectivityManager; @@ -49,14 +50,12 @@ import android.test.suitebuilder.annotation.LargeTest; import android.test.suitebuilder.annotation.Suppress; import android.text.format.DateUtils; -import com.android.providers.downloads.Constants; -import com.android.providers.downloads.DownloadReceiver; +import libcore.io.IoUtils; + import com.google.mockwebserver.MockResponse; import com.google.mockwebserver.RecordedRequest; import com.google.mockwebserver.SocketPolicy; -import libcore.io.IoUtils; - import java.io.File; import java.io.FileInputStream; import java.io.FileNotFoundException; @@ -83,10 +82,8 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest { protected void setUp() throws Exception { super.setUp(); - mNotifManager = (NotificationManager) getContext() - .getSystemService(Context.NOTIFICATION_SERVICE); - mDownloadManager = (DownloadManager) getContext() - .getSystemService(Context.DOWNLOAD_SERVICE); + mNotifManager = getContext().getSystemService(NotificationManager.class); + mDownloadManager = getContext().getSystemService(DownloadManager.class); mTestDirectory = new File(Environment.getExternalStorageDirectory() + File.separator + "download_manager_functional_test"); @@ -398,10 +395,12 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest { mSystemFacade.mMaxBytesOverMobile = (long) FILE_CONTENT.length() - 1; mSystemFacade.mActiveNetworkType = ConnectivityManager.TYPE_MOBILE; + mSystemFacade.mIsMetered = true; Download download = enqueueRequest(getRequest()); download.runUntilStatus(DownloadManager.STATUS_PAUSED); mSystemFacade.mActiveNetworkType = ConnectivityManager.TYPE_WIFI; + mSystemFacade.mIsMetered = false; // first response was read, but aborted after the DL manager processed the Content-Length // header, so we need to enqueue a second one download.runUntilStatus(DownloadManager.STATUS_SUCCESSFUL); @@ -544,7 +543,7 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest { Download download = enqueueRequest(getRequest()); DownloadReceiver receiver = new DownloadReceiver(); - receiver.mSystemFacade = mSystemFacade; + Helpers.setSystemFacade(mSystemFacade); Intent intent = new Intent(Constants.ACTION_LIST); intent.setData(Uri.parse(Downloads.Impl.CONTENT_URI + "/" + download.mId)); intent.putExtra(DownloadManager.EXTRA_NOTIFICATION_CLICK_DOWNLOAD_IDS, @@ -561,7 +560,7 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest { Download download = enqueueRequest(getRequest()); DownloadReceiver receiver = new DownloadReceiver(); - receiver.mSystemFacade = mSystemFacade; + Helpers.setSystemFacade(mSystemFacade); Intent intent = new Intent(Constants.ACTION_CANCEL); intent.setData(Uri.parse(Downloads.Impl.CONTENT_URI + "/" + download.mId)); @@ -592,6 +591,7 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest { enqueueResponse(buildEmptyResponse(HTTP_OK)); mSystemFacade.mActiveNetworkType = ConnectivityManager.TYPE_MOBILE; + mSystemFacade.mIsMetered = true; // by default, use any connection Download download = enqueueRequest(getRequest()); @@ -603,6 +603,7 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest { download.runUntilStatus(DownloadManager.STATUS_PAUSED); // ...then enable wifi mSystemFacade.mActiveNetworkType = ConnectivityManager.TYPE_WIFI; + mSystemFacade.mIsMetered = false; download.runUntilStatus(DownloadManager.STATUS_SUCCESSFUL); } @@ -632,6 +633,7 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest { assertTrue(mResolver.mNotifyWasCalled); } + @Suppress public void testNotificationNever() throws Exception { enqueueResponse(buildEmptyResponse(HTTP_OK)); @@ -639,10 +641,11 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest { getRequest().setNotificationVisibility(DownloadManager.Request.VISIBILITY_HIDDEN)); download.runUntilStatus(DownloadManager.STATUS_SUCCESSFUL); - verify(mNotifManager, times(1)).cancelAll(); + // TODO: verify different notif types with tags verify(mNotifManager, never()).notify(anyString(), anyInt(), isA(Notification.class)); } + @Suppress public void testNotificationVisible() throws Exception { enqueueResponse(buildEmptyResponse(HTTP_OK)); @@ -651,10 +654,10 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest { download.runUntilStatus(DownloadManager.STATUS_SUCCESSFUL); // TODO: verify different notif types with tags - verify(mNotifManager, times(1)).cancelAll(); verify(mNotifManager, atLeastOnce()).notify(anyString(), anyInt(), isA(Notification.class)); } + @Suppress public void testNotificationVisibleComplete() throws Exception { enqueueResponse(buildEmptyResponse(HTTP_OK)); @@ -663,7 +666,6 @@ public class PublicApiFunctionalTest extends AbstractPublicApiTest { download.runUntilStatus(DownloadManager.STATUS_SUCCESSFUL); // TODO: verify different notif types with tags - verify(mNotifManager, times(1)).cancelAll(); verify(mNotifManager, atLeastOnce()).notify(anyString(), anyInt(), isA(Notification.class)); } diff --git a/tests/src/com/android/providers/downloads/ThreadingTest.java b/tests/src/com/android/providers/downloads/ThreadingTest.java index 1e501444..dda4db5e 100644 --- a/tests/src/com/android/providers/downloads/ThreadingTest.java +++ b/tests/src/com/android/providers/downloads/ThreadingTest.java @@ -46,19 +46,6 @@ public class ThreadingTest extends AbstractPublicApiTest { super.tearDown(); } - /** - * Test for race conditions when the service is flooded with startService() calls while running - * a download. - */ - public void testFloodServiceWithStarts() throws Exception { - enqueueResponse(buildResponse(HTTP_OK, FILE_CONTENT)); - Download download = enqueueRequest(getRequest()); - while (download.getStatus() != DownloadManager.STATUS_SUCCESSFUL) { - startService(null); - Thread.sleep(10); - } - } - public void testFilenameRace() throws Exception { final List> downloads = Lists.newArrayList(); final HashSet expectedBodies = Sets.newHashSet(); @@ -73,12 +60,10 @@ public class ThreadingTest extends AbstractPublicApiTest { final Download d = enqueueRequest(getRequest()); downloads.add(Pair.create(d, body)); expectedBodies.add(body); + startDownload(d.mId); } - // Kick off downloads in parallel final long startMillis = mSystemFacade.currentTimeMillis(); - startService(null); - for (Pair d : downloads) { d.first.waitForStatus(DownloadManager.STATUS_SUCCESSFUL, startMillis); } -- cgit v1.2.3