Stick FileOperationService to foreground.
Note there is still a small window between service is started and our
service enters foreground mode because originally we only show
notifications once job has started. Therefore our service is still a
background service before the first job starts to set up.
Bug: 34512210
Test: Automatic tests pass. Some manual tests.
Change-Id: I33987c1da861bb7583d12a21547f44c77de860d8
(cherry picked from commit 0bed4ce4cfc5b40fd2027681be1f498135e6486d)
(cherry picked from commit 8167ebe40e6c81e2f23df05e391d38fd984b2bb6)
diff --git a/src/com/android/documentsui/services/FileOperationService.java b/src/com/android/documentsui/services/FileOperationService.java
index d9745f0..6965a94 100644
--- a/src/com/android/documentsui/services/FileOperationService.java
+++ b/src/com/android/documentsui/services/FileOperationService.java
@@ -19,6 +19,7 @@
import static com.android.documentsui.base.Shared.DEBUG;
import android.annotation.IntDef;
+import android.app.Notification;
import android.app.NotificationManager;
import android.app.Service;
import android.content.Intent;
@@ -37,12 +38,14 @@
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicReference;
import javax.annotation.concurrent.GuardedBy;
public class FileOperationService extends Service implements Job.Listener {
private static final int POOL_SIZE = 2; // "pool size", not *max* "pool size".
+
private static final int NOTIFICATION_ID_PROGRESS = 0;
private static final int NOTIFICATION_ID_FAILURE = 1;
private static final int NOTIFICATION_ID_WARNING = 2;
@@ -95,12 +98,20 @@
// Use a handler to schedule monitor tasks.
@VisibleForTesting Handler handler;
- @GuardedBy("mRunning")
- private final Map<String, JobRecord> mRunning = new HashMap<>();
+ // Use a foreground manager to change foreground state of this service.
+ @VisibleForTesting ForegroundManager foregroundManager;
+
+ // Use a notification manager to post and cancel notifications for jobs.
+ @VisibleForTesting NotificationManager notificationManager;
+
+ @GuardedBy("mJobs")
+ private final Map<String, JobRecord> mJobs = new HashMap<>();
+
+ // The job whose notification is used to keep the service in foreground mode.
+ private final AtomicReference<Job> mForegroundJob = new AtomicReference<>();
private PowerManager mPowerManager;
private PowerManager.WakeLock mWakeLock; // the wake lock, if held.
- private NotificationManager mNotificationManager;
private int mLastServiceId;
@@ -120,9 +131,16 @@
handler = new Handler();
}
+ if (foregroundManager == null) {
+ foregroundManager = createForegroundManager(this);
+ }
+
+ if (notificationManager == null) {
+ notificationManager = getSystemService(NotificationManager.class);
+ }
+
if (DEBUG) Log.d(TAG, "Created.");
mPowerManager = getSystemService(PowerManager.class);
- mNotificationManager = getSystemService(NotificationManager.class);
}
@Override
@@ -170,12 +188,12 @@
}
private void handleOperation(String jobId, FileOperation operation) {
- synchronized (mRunning) {
+ synchronized (mJobs) {
if (mWakeLock == null) {
mWakeLock = mPowerManager.newWakeLock(PowerManager.PARTIAL_WAKE_LOCK, TAG);
}
- if (mRunning.containsKey(jobId)) {
+ if (mJobs.containsKey(jobId)) {
Log.w(TAG, "Duplicate job id: " + jobId
+ ". Ignoring job request for operation: " + operation + ".");
return;
@@ -190,10 +208,10 @@
assert (job != null);
if (DEBUG) Log.d(TAG, "Scheduling job " + job.id + ".");
Future<?> future = getExecutorService(operation.getOpType()).submit(job);
- mRunning.put(jobId, new JobRecord(job, future));
+ mJobs.put(jobId, new JobRecord(job, future));
// Acquire wake lock to keep CPU running until we finish all jobs. Acquire wake lock
- // after we create a job and put it in mRunning to avoid potential leaking of wake lock
+ // after we create a job and put it in mJobs to avoid potential leaking of wake lock
// in case where job creation fails.
mWakeLock.acquire();
}
@@ -212,12 +230,12 @@
if (DEBUG) Log.d(TAG, "handleCancel: " + jobId);
- synchronized (mRunning) {
+ synchronized (mJobs) {
// Do nothing if the cancelled ID doesn't match the current job ID. This prevents racey
// cancellation requests from affecting unrelated copy jobs. However, if the current job ID
// is null, the service most likely crashed and was revived by the incoming cancel intent.
// In that case, always allow the cancellation to proceed.
- JobRecord record = mRunning.get(jobId);
+ JobRecord record = mJobs.get(jobId);
if (record != null) {
record.job.cancel();
}
@@ -227,7 +245,7 @@
// interactivity for the user in case the copy loop is stalled.
// Try to cancel it even if we don't have a job id...in case there is some sad
// orphan notification.
- mNotificationManager.cancel(jobId, NOTIFICATION_ID_PROGRESS);
+ notificationManager.cancel(jobId, NOTIFICATION_ID_PROGRESS);
// TODO: Guarantee the job is being finalized
}
@@ -246,7 +264,7 @@
}
}
- @GuardedBy("mRunning")
+ @GuardedBy("mJobs")
private void deleteJob(Job job) {
if (DEBUG) Log.d(TAG, "deleteJob: " + job.id);
@@ -256,13 +274,12 @@
mWakeLock = null;
}
- JobRecord record = mRunning.remove(job.id);
+ JobRecord record = mJobs.remove(job.id);
assert(record != null);
record.job.cleanup();
- if (mRunning.isEmpty()) {
- shutdown();
- }
+ // Delay the shutdown until we've cleaned up all notifications. shutdown() is now posted in
+ // onFinished(Job job) to main thread.
}
/**
@@ -292,12 +309,20 @@
public void onStart(Job job) {
if (DEBUG) Log.d(TAG, "onStart: " + job.id);
+ Notification notification = job.getSetupNotification();
+ // If there is no foreground job yet, set this job to foreground job.
+ if (mForegroundJob.compareAndSet(null, job)) {
+ if (DEBUG) Log.d(TAG, "Set foreground job to " + job.id);
+ foregroundManager.startForeground(NOTIFICATION_ID_PROGRESS, notification);
+ }
+
// Show start up notification
- mNotificationManager.notify(
- job.id, NOTIFICATION_ID_PROGRESS, job.getSetupNotification());
+ if (DEBUG) Log.d(TAG, "Posting notification for " + job.id);
+ notificationManager.notify(
+ job.id, NOTIFICATION_ID_PROGRESS, notification);
// Set up related monitor
- JobMonitor monitor = new JobMonitor(job, mNotificationManager, handler);
+ JobMonitor monitor = new JobMonitor(job, notificationManager, handler);
monitor.start();
}
@@ -306,32 +331,75 @@
assert(job.isFinished());
if (DEBUG) Log.d(TAG, "onFinished: " + job.id);
- // Use the same thread of monitors to tackle notifications to avoid race conditions.
- // Otherwise we may fail to dismiss progress notification.
- handler.post(() -> {
- // Dismiss the ongoing copy notification when the copy is done.
- mNotificationManager.cancel(job.id, NOTIFICATION_ID_PROGRESS);
-
- if (job.hasFailures()) {
- if (!job.failedUris.isEmpty()) {
- Log.e(TAG, "Job failed to resolve uris: " + job.failedUris + ".");
- }
- if (!job.failedDocs.isEmpty()) {
- Log.e(TAG, "Job failed to process docs: " + job.failedDocs + ".");
- }
- mNotificationManager.notify(
- job.id, NOTIFICATION_ID_FAILURE, job.getFailureNotification());
- }
-
- if (job.hasWarnings()) {
- if (DEBUG) Log.d(TAG, "Job finished with warnings.");
- mNotificationManager.notify(
- job.id, NOTIFICATION_ID_WARNING, job.getWarningNotification());
- }
- });
-
- synchronized (mRunning) {
+ synchronized (mJobs) {
+ // Delete the job from mJobs first to avoid this job being selected as the foreground
+ // task again if we need to swap the foreground job.
deleteJob(job);
+
+ // Update foreground state before cleaning up notification. If the finishing job is the
+ // foreground job, we would need to switch to another one or go to background before
+ // we can clean up notifications.
+ updateForegroundState(job);
+
+ // Use the same thread of monitors to tackle notifications to avoid race conditions.
+ // Otherwise we may fail to dismiss progress notification.
+ handler.post(() -> cleanUpNotification(job));
+
+ // Post the shutdown message to main thread after cleanUpNotification() to give it a
+ // chance to run. Otherwise this process may be torn down by Android before we've
+ // cleaned up the notifications of the last job.
+ if (mJobs.isEmpty()) {
+ handler.post(this::shutdown);
+ }
+ }
+ }
+
+ @GuardedBy("mJobs")
+ private void updateForegroundState(Job job) {
+ Job candidate = mJobs.isEmpty() ? null : mJobs.values().iterator().next().job;
+
+ // If foreground job is retiring and there is still work to do, we need to set it to a new
+ // job.
+ if (mForegroundJob.compareAndSet(job, candidate)) {
+ if (candidate == null) {
+ if (DEBUG) Log.d(TAG, "Stop foreground");
+ // Remove the notification here just in case we're torn down before we have the
+ // chance to clean up notifications.
+ foregroundManager.stopForeground(true);
+ } else {
+ if (DEBUG) Log.d(TAG, "Switch foreground job to " + candidate.id);
+
+ Notification notification = (candidate.getState() == Job.STATE_STARTED)
+ ? candidate.getSetupNotification()
+ : candidate.getProgressNotification();
+ foregroundManager.startForeground(NOTIFICATION_ID_PROGRESS, notification);
+ notificationManager.notify(candidate.id, NOTIFICATION_ID_PROGRESS,
+ notification);
+ }
+ }
+ }
+
+ private void cleanUpNotification(Job job) {
+
+ if (DEBUG) Log.d(TAG, "Canceling notification for " + job.id);
+ // Dismiss the ongoing copy notification when the copy is done.
+ notificationManager.cancel(job.id, NOTIFICATION_ID_PROGRESS);
+
+ if (job.hasFailures()) {
+ if (!job.failedUris.isEmpty()) {
+ Log.e(TAG, "Job failed to resolve uris: " + job.failedUris + ".");
+ }
+ if (!job.failedDocs.isEmpty()) {
+ Log.e(TAG, "Job failed to process docs: " + job.failedDocs + ".");
+ }
+ notificationManager.notify(
+ job.id, NOTIFICATION_ID_FAILURE, job.getFailureNotification());
+ }
+
+ if (job.hasWarnings()) {
+ if (DEBUG) Log.d(TAG, "Job finished with warnings.");
+ notificationManager.notify(
+ job.id, NOTIFICATION_ID_WARNING, job.getWarningNotification());
}
}
@@ -391,4 +459,24 @@
public IBinder onBind(Intent intent) {
return null; // Boilerplate. See super#onBind
}
+
+ private static ForegroundManager createForegroundManager(final Service service) {
+ return new ForegroundManager() {
+ @Override
+ public void startForeground(int id, Notification notification) {
+ service.startForeground(id, notification);
+ }
+
+ @Override
+ public void stopForeground(boolean removeNotification) {
+ service.stopForeground(removeNotification);
+ }
+ };
+ }
+
+ @VisibleForTesting
+ interface ForegroundManager {
+ void startForeground(int id, Notification notification);
+ void stopForeground(boolean removeNotification);
+ }
}
diff --git a/tests/common/com/android/documentsui/services/TestForegroundManager.java b/tests/common/com/android/documentsui/services/TestForegroundManager.java
new file mode 100644
index 0000000..f4ba1c1
--- /dev/null
+++ b/tests/common/com/android/documentsui/services/TestForegroundManager.java
@@ -0,0 +1,56 @@
+/*
+ * Copyright (C) 2017 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.documentsui.services;
+
+import static junit.framework.Assert.assertNotNull;
+import static junit.framework.Assert.assertNull;
+
+import android.app.Notification;
+
+class TestForegroundManager implements FileOperationService.ForegroundManager {
+
+ private int mForegroundId = -1;
+ private Notification mForegroundNotification;
+
+ @Override
+ public void startForeground(int id, Notification notification) {
+ mForegroundId = id;
+ mForegroundNotification = notification;
+ }
+
+ @Override
+ public void stopForeground(boolean cancelNotification) {
+ mForegroundId = -1;
+ mForegroundNotification = null;
+ }
+
+ void assertInForeground() {
+ assertNotNull(mForegroundNotification);
+ }
+
+ void assertInBackground() {
+ assertNull(mForegroundNotification);
+ }
+
+ int getForegroundId() {
+ return mForegroundId;
+ }
+
+ Notification getForegroundNotification() {
+ return mForegroundNotification;
+ }
+}
diff --git a/tests/common/com/android/documentsui/services/TestNotificationManager.java b/tests/common/com/android/documentsui/services/TestNotificationManager.java
new file mode 100644
index 0000000..463f2d4
--- /dev/null
+++ b/tests/common/com/android/documentsui/services/TestNotificationManager.java
@@ -0,0 +1,103 @@
+/*
+ * Copyright (C) 2017 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.documentsui.services;
+
+import static junit.framework.Assert.assertEquals;
+import static junit.framework.Assert.assertNotNull;
+import static junit.framework.Assert.assertTrue;
+
+import android.app.Notification;
+import android.app.NotificationManager;
+import android.util.SparseArray;
+
+import org.mockito.Mockito;
+import org.mockito.invocation.InvocationOnMock;
+import org.mockito.stubbing.Answer;
+
+import java.util.HashMap;
+
+class TestNotificationManager {
+
+ private final TestForegroundManager mForegroundManager;
+ private final SparseArray<HashMap<String, Notification>> mNotifications = new SparseArray<>();
+ private final Answer<Void> mAnswer = this::invoke;
+
+ TestNotificationManager(TestForegroundManager foregroundManager) {
+ assert(foregroundManager != null);
+ mForegroundManager = foregroundManager;
+ }
+
+ private void notify(String tag, int id, Notification notification) {
+ if (notification == mForegroundManager.getForegroundNotification()
+ && id != mForegroundManager.getForegroundId()) {
+ throw new IllegalStateException("Mismatching ID and notification.");
+ }
+
+ if (mNotifications.get(id) == null) {
+ mNotifications.put(id, new HashMap<>());
+ }
+
+ mNotifications.get(id).put(tag, notification);
+ }
+
+ private void cancel(String tag, int id) {
+ final HashMap<String, Notification> idMap = mNotifications.get(id);
+ if (idMap != null && idMap.containsKey(tag)) {
+ final Notification notification = idMap.get(tag);
+ // Only cancel non-foreground notification
+ if (mForegroundManager.getForegroundNotification() != notification) {
+ idMap.remove(tag);
+ }
+ }
+ }
+
+ private Void invoke(InvocationOnMock invocation) {
+ Object[] args = invocation.getArguments();
+ switch (invocation.getMethod().getName()) {
+ case "notify":
+ if (args.length == 2) {
+ notify(null, (Integer) args[0], (Notification) args[1]);
+ }
+ if (args.length == 3) {
+ notify((String) args[0], (Integer) args[1], (Notification) args[2]);
+ }
+ break;
+ case "cancel":
+ if (args.length == 1) {
+ cancel(null, (Integer) args[0]);
+ }
+ if (args.length == 2) {
+ cancel((String) args[0], (Integer) args[1]);
+ }
+ break;
+ }
+ return null;
+ }
+
+ NotificationManager createNotificationManager() {
+ return Mockito.mock(NotificationManager.class, mAnswer);
+ }
+
+ void assertNumberOfNotifications(int expect) {
+ int count = 0;
+ for (int i = 0; i < mNotifications.size(); ++i) {
+ count += mNotifications.valueAt(i).size();
+ }
+
+ assertEquals(expect, count);
+ }
+}
diff --git a/tests/common/com/android/documentsui/testing/TestHandler.java b/tests/common/com/android/documentsui/testing/TestHandler.java
index 143ec71..c458c9b 100644
--- a/tests/common/com/android/documentsui/testing/TestHandler.java
+++ b/tests/common/com/android/documentsui/testing/TestHandler.java
@@ -19,6 +19,7 @@
import android.os.Handler;
import android.os.Looper;
import android.os.Message;
+import android.os.SystemClock;
import java.util.TimerTask;
@@ -28,6 +29,14 @@
public class TestHandler extends Handler {
private TestTimer mTimer = new TestTimer();
+ // Handler uses SystemClock.uptimeMillis() when scheduling task to get current time, but
+ // TestTimer has its own warped time for us to "fast forward" into the future. Therefore after
+ // we "fast forwarded" TestTimer once Handler may schedule tasks running in the "past" relative
+ // to the fast-forwarded TestTimer and cause problems. This value is used to track how much we
+ // fast-forward into the future to make sure we schedule tasks in the future of TestTimer as
+ // well.
+ private long mTimeDelta = 0;
+
public TestHandler() {
// Use main looper to trick underlying handler, we're not using it at all.
super(Looper.getMainLooper());
@@ -39,11 +48,19 @@
public void dispatchNextMessage() {
mTimer.fastForwardToNextTask();
+
+ mTimeDelta = mTimer.getNow() - SystemClock.uptimeMillis();
+ }
+
+ public void dispatchAllScheduledMessages() {
+ while (hasScheduledMessage()) {
+ dispatchNextMessage();
+ }
}
public void dispatchAllMessages() {
while (hasScheduledMessage()) {
- dispatchNextMessage();
+ dispatchAllScheduledMessages();
}
}
@@ -51,7 +68,7 @@
public boolean sendMessageAtTime(Message msg, long uptimeMillis) {
msg.setTarget(this);
TimerTask task = new MessageTimerTask(msg);
- mTimer.scheduleAtTime(new TestTimer.Task(task), uptimeMillis);
+ mTimer.scheduleAtTime(new TestTimer.Task(task), uptimeMillis + mTimeDelta);
return true;
}
diff --git a/tests/common/com/android/documentsui/testing/TestScheduledExecutorService.java b/tests/common/com/android/documentsui/testing/TestScheduledExecutorService.java
index 7bfd1c2..bc05d4c 100644
--- a/tests/common/com/android/documentsui/testing/TestScheduledExecutorService.java
+++ b/tests/common/com/android/documentsui/testing/TestScheduledExecutorService.java
@@ -150,6 +150,8 @@
public void run(int taskIndex) {
scheduled.get(taskIndex).runnable.run();
+
+ scheduled.remove(taskIndex);
}
public void assertAlive() {
diff --git a/tests/common/com/android/documentsui/testing/TestTimer.java b/tests/common/com/android/documentsui/testing/TestTimer.java
index 04af283..e1a6610 100644
--- a/tests/common/com/android/documentsui/testing/TestTimer.java
+++ b/tests/common/com/android/documentsui/testing/TestTimer.java
@@ -32,6 +32,10 @@
private final LinkedList<Task> mTaskList = new LinkedList<>();
+ public long getNow() {
+ return mNow;
+ }
+
public void fastForwardTo(long time) {
if (time < mNow) {
throw new IllegalArgumentException("Can't fast forward to past.");
diff --git a/tests/unit/com/android/documentsui/services/FileOperationServiceTest.java b/tests/unit/com/android/documentsui/services/FileOperationServiceTest.java
index 56ddae8..82b8bbe 100644
--- a/tests/unit/com/android/documentsui/services/FileOperationServiceTest.java
+++ b/tests/unit/com/android/documentsui/services/FileOperationServiceTest.java
@@ -22,6 +22,7 @@
import static com.android.documentsui.services.FileOperations.createJobId;
import static com.google.android.collect.Lists.newArrayList;
+import android.app.Notification;
import android.content.Context;
import android.content.Intent;
import android.net.Uri;
@@ -58,6 +59,8 @@
private TestScheduledExecutorService mExecutor;
private TestScheduledExecutorService mDeletionExecutor;
private TestHandler mHandler;
+ private TestForegroundManager mForegroundManager;
+ private TestNotificationManager mTestNotificationManager;
public FileOperationServiceTest() {
super(FileOperationService.class);
@@ -71,6 +74,8 @@
mExecutor = new TestScheduledExecutorService();
mDeletionExecutor = new TestScheduledExecutorService();
mHandler = new TestHandler();
+ mForegroundManager = new TestForegroundManager();
+ mTestNotificationManager = new TestNotificationManager(mForegroundManager);
mCopyJobs.clear();
mDeleteJobs.clear();
@@ -86,6 +91,12 @@
assertNull(mService.handler);
mService.handler = mHandler;
+
+ assertNull(mService.foregroundManager);
+ mService.foregroundManager = mForegroundManager;
+
+ assertNull(mService.notificationManager);
+ mService.notificationManager = mTestNotificationManager.createNotificationManager();
}
@Override
@@ -96,9 +107,7 @@
// There are lots of progress notifications generated in this test case.
// Dismiss all of them here.
- while (mHandler.hasScheduledMessage()) {
- mHandler.dispatchAllMessages();
- }
+ mHandler.dispatchAllMessages();
}
public void testRunsCopyJobs() throws Exception {
@@ -238,6 +247,42 @@
assertExecutorsShutdown();
}
+ public void testRunsInForeground_MultipleJobs() throws Exception {
+ startService(createCopyIntent(newArrayList(ALPHA_DOC), BETA_DOC));
+ startService(createCopyIntent(newArrayList(GAMMA_DOC), DELTA_DOC));
+
+ mExecutor.run(0);
+ mForegroundManager.assertInForeground();
+
+ mHandler.dispatchAllMessages();
+ mForegroundManager.assertInForeground();
+ }
+
+ public void testFinishesInBackground_MultipleJobs() throws Exception {
+ startService(createCopyIntent(newArrayList(ALPHA_DOC), BETA_DOC));
+ startService(createCopyIntent(newArrayList(GAMMA_DOC), DELTA_DOC));
+
+ mExecutor.run(0);
+ mForegroundManager.assertInForeground();
+
+ mHandler.dispatchAllMessages();
+ mForegroundManager.assertInForeground();
+
+ mExecutor.run(0);
+ mHandler.dispatchAllMessages();
+ mForegroundManager.assertInBackground();
+ }
+
+ public void testAllNotificationsDismissedAfterShutdown() throws Exception {
+ startService(createCopyIntent(newArrayList(ALPHA_DOC), BETA_DOC));
+ startService(createCopyIntent(newArrayList(GAMMA_DOC), DELTA_DOC));
+
+ mExecutor.runAll();
+
+ mHandler.dispatchAllMessages();
+ mTestNotificationManager.assertNumberOfNotifications(0);
+ }
+
private Intent createCopyIntent(ArrayList<DocumentInfo> files, DocumentInfo dest)
throws Exception {
DocumentStack stack = new DocumentStack();