Add WorkerWrapper interruption; remove usage of Futures.
This is part 1 of removing ExecutorService and using
Executor instead. Here we add an explicit interrupt()
method to WorkerWrapper and check it when deciding if
the work finished successfully.
Also cleaned up a broken state where ALL non-cancelled
states could lead to handleResult being called.
Removed blocking cancellation in WorkManagerImplTests
due to thread contention issues because of synchronous
execution rules.
Removed Worker.isInterrupted. Need to come up with a
suitable alternative.
Removed Processor.stopWork's second argument (may
interrupt thread) because it's not relevant anymore.
Test: Added and ran tests.
Change-Id: I28de5081e8d30c3d9147b1d74008ab931963fa81
diff --git a/work/workmanager-firebase/src/androidTest/java/androidx/work/worker/FirebaseInfiniteTestWorker.java b/work/workmanager-firebase/src/androidTest/java/androidx/work/worker/FirebaseInfiniteTestWorker.java
index 716d8eb..72918da 100644
--- a/work/workmanager-firebase/src/androidTest/java/androidx/work/worker/FirebaseInfiniteTestWorker.java
+++ b/work/workmanager-firebase/src/androidTest/java/androidx/work/worker/FirebaseInfiniteTestWorker.java
@@ -17,7 +17,6 @@
package androidx.work.worker;
import android.support.annotation.NonNull;
-import android.util.Log;
import androidx.work.Worker;
@@ -31,10 +30,6 @@
@Override
public @NonNull WorkerResult doWork() {
while (true) {
- if (isInterrupted()) {
- Log.e(TAG, "Interrupted");
- return WorkerResult.RETRY;
- }
}
}
}
diff --git a/work/workmanager/src/androidTest/java/androidx/work/impl/ProcessorTest.java b/work/workmanager/src/androidTest/java/androidx/work/impl/ProcessorTest.java
index ef87ee2..1b72e55 100644
--- a/work/workmanager/src/androidTest/java/androidx/work/impl/ProcessorTest.java
+++ b/work/workmanager/src/androidTest/java/androidx/work/impl/ProcessorTest.java
@@ -55,8 +55,7 @@
@SmallTest
public void testStopWork_invalidWorkId() {
String id = "INVALID_WORK_ID";
- assertThat(mProcessor.stopWork(id, true), is(false));
- assertThat(mProcessor.stopWork(id, false), is(false));
+ assertThat(mProcessor.stopWork(id), is(false));
}
@Test
diff --git a/work/workmanager/src/androidTest/java/androidx/work/impl/WorkManagerImplTest.java b/work/workmanager/src/androidTest/java/androidx/work/impl/WorkManagerImplTest.java
index a344435..78d295a 100644
--- a/work/workmanager/src/androidTest/java/androidx/work/impl/WorkManagerImplTest.java
+++ b/work/workmanager/src/androidTest/java/androidx/work/impl/WorkManagerImplTest.java
@@ -140,10 +140,6 @@
@After
public void tearDown() {
- List<String> ids = mDatabase.workSpecDao().getAllWorkSpecIds();
- for (String id : ids) {
- mWorkManagerImpl.blocking().cancelWorkByIdBlocking(id);
- }
WorkManagerImpl.setDelegate(null);
ArchTaskExecutor.getInstance().setDelegate(null);
}
diff --git a/work/workmanager/src/androidTest/java/androidx/work/impl/WorkerWrapperTest.java b/work/workmanager/src/androidTest/java/androidx/work/impl/WorkerWrapperTest.java
index e4d4f14..b04a186 100644
--- a/work/workmanager/src/androidTest/java/androidx/work/impl/WorkerWrapperTest.java
+++ b/work/workmanager/src/androidTest/java/androidx/work/impl/WorkerWrapperTest.java
@@ -631,4 +631,20 @@
verify(mMockListener).onExecuted(work.getId(), false, false);
assertThat(mWorkSpecDao.getState(work.getId()), is(FAILED));
}
+
+ @Test
+ @SmallTest
+ public void testInterruption() throws InterruptedException {
+ WorkRequest work = new WorkRequest.Builder(TestWorker.class).build();
+ insertWork(work);
+
+ WorkerWrapper workerWrapper = new WorkerWrapper.Builder(mContext, mDatabase, work.getId())
+ .withSchedulers(Collections.singletonList(mMockScheduler))
+ .withListener(mMockListener)
+ .build();
+ Executors.newSingleThreadExecutor().submit(workerWrapper);
+ workerWrapper.interrupt();
+ Thread.sleep(6000L);
+ verify(mMockListener).onExecuted(work.getId(), false, true);
+ }
}
diff --git a/work/workmanager/src/androidTest/java/androidx/work/worker/InfiniteTestWorker.java b/work/workmanager/src/androidTest/java/androidx/work/worker/InfiniteTestWorker.java
index ee00298..a16beaf 100644
--- a/work/workmanager/src/androidTest/java/androidx/work/worker/InfiniteTestWorker.java
+++ b/work/workmanager/src/androidTest/java/androidx/work/worker/InfiniteTestWorker.java
@@ -17,7 +17,6 @@
package androidx.work.worker;
import android.support.annotation.NonNull;
-import android.util.Log;
import androidx.work.Worker;
@@ -26,15 +25,10 @@
*/
public class InfiniteTestWorker extends Worker {
- private static final String TAG = "InfiniteTestWorker";
@Override
public @NonNull WorkerResult doWork() {
while (true) {
- if (isInterrupted()) {
- Log.e(TAG, "Interrupted");
- return WorkerResult.RETRY;
- }
}
}
}
diff --git a/work/workmanager/src/androidTest/java/androidx/work/worker/LongRunningWorker.java b/work/workmanager/src/androidTest/java/androidx/work/worker/LongRunningWorker.java
new file mode 100644
index 0000000..a2f2412
--- /dev/null
+++ b/work/workmanager/src/androidTest/java/androidx/work/worker/LongRunningWorker.java
@@ -0,0 +1,32 @@
+/*
+ * Copyright 2018 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 androidx.work.worker;
+
+import androidx.work.Worker;
+
+public class LongRunningWorker extends Worker {
+
+ @Override
+ public WorkerResult doWork() {
+ try {
+ Thread.sleep(5000L);
+ } catch (InterruptedException e) {
+ // Do nothing.
+ }
+ return WorkerResult.SUCCESS;
+ }
+}
diff --git a/work/workmanager/src/main/java/androidx/work/Worker.java b/work/workmanager/src/main/java/androidx/work/Worker.java
index 3f6225e..b26c970 100644
--- a/work/workmanager/src/main/java/androidx/work/Worker.java
+++ b/work/workmanager/src/main/java/androidx/work/Worker.java
@@ -123,18 +123,6 @@
}
/**
- * Determines if the {@link Worker} was interrupted and should stop executing.
- * The {@link Worker} can be interrupted for the following reasons:
- * 1. The {@link WorkRequest} or {@link PeriodicWorkRequest} was explicitly cancelled.
- * {@link WorkManager#cancelAllWorkByTag(String)}
- * 2. Constraints set in {@link WorkRequest} or {@link PeriodicWorkRequest} are no longer valid.
- * @return {@code true} if {@link Worker} is instructed to stop executing.
- */
- protected final boolean isInterrupted() {
- return Thread.currentThread().isInterrupted();
- }
-
- /**
* @hide
*/
@RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
diff --git a/work/workmanager/src/main/java/androidx/work/impl/Processor.java b/work/workmanager/src/main/java/androidx/work/impl/Processor.java
index 306f493..0887915 100644
--- a/work/workmanager/src/main/java/androidx/work/impl/Processor.java
+++ b/work/workmanager/src/main/java/androidx/work/impl/Processor.java
@@ -27,7 +27,6 @@
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutorService;
-import java.util.concurrent.Future;
/**
* A Processor can intelligently schedule and execute work on demand.
@@ -41,7 +40,7 @@
private Context mAppContext;
private WorkDatabase mWorkDatabase;
- private Map<String, Future<?>> mEnqueuedWorkMap;
+ private Map<String, WorkerWrapper> mEnqueuedWorkMap;
private List<Scheduler> mSchedulers;
private ExecutorService mExecutorService;
@@ -93,7 +92,8 @@
.withSchedulers(mSchedulers)
.withRuntimeExtras(runtimeExtras)
.build();
- mEnqueuedWorkMap.put(id, mExecutorService.submit(workWrapper));
+ mEnqueuedWorkMap.put(id, workWrapper);
+ mExecutorService.submit(workWrapper);
Log.d(TAG, String.format("%s: processing %s", getClass().getSimpleName(), id));
return true;
}
@@ -102,36 +102,23 @@
* Tries to stop a unit of work.
*
* @param id The work id to stop
- * @param mayInterruptIfRunning If {@code true}, we try to interrupt the {@link Future} if it's
- * running
* @return {@code true} if the work was stopped successfully
*/
- public synchronized boolean stopWork(String id, boolean mayInterruptIfRunning) {
- Log.d(TAG,
- String.format("%s canceling %s; mayInterruptIfRunning = %s",
- getClass().getSimpleName(),
- id,
- mayInterruptIfRunning));
- Future<?> future = mEnqueuedWorkMap.get(id);
- if (future != null) {
- boolean cancelled = future.cancel(mayInterruptIfRunning);
- if (cancelled) {
- mEnqueuedWorkMap.remove(id);
- Log.d(TAG, String.format("Future successfully canceled for %s", id));
- } else {
- Log.d(TAG, String.format("Future could not be canceled for %s", id));
- }
- return cancelled;
- } else {
- Log.d(TAG, String.format("%s future could not be found for %s",
- getClass().getSimpleName(), id));
+ public synchronized boolean stopWork(String id) {
+ Log.d(TAG, String.format("Processor cancelling %s", id));
+ WorkerWrapper wrapper = mEnqueuedWorkMap.remove(id);
+ if (wrapper != null) {
+ wrapper.interrupt();
+ Log.d(TAG, String.format("WorkerWrapper interrupted for %s", id));
+ return true;
}
+ Log.d(TAG, String.format("WorkerWrapper could not be found for %s", id));
return false;
}
/**
* Sets the given {@code id} as cancelled. This does not actually stop any processing; call
- * {@link #stopWork(String, boolean)} to do that.
+ * {@link #stopWork(String)} to do that.
*
* @param id The work id to mark as cancelled
*/
diff --git a/work/workmanager/src/main/java/androidx/work/impl/WorkerWrapper.java b/work/workmanager/src/main/java/androidx/work/impl/WorkerWrapper.java
index c22ef36..59217f3 100644
--- a/work/workmanager/src/main/java/androidx/work/impl/WorkerWrapper.java
+++ b/work/workmanager/src/main/java/androidx/work/impl/WorkerWrapper.java
@@ -65,6 +65,8 @@
private WorkSpecDao mWorkSpecDao;
private DependencyDao mDependencyDao;
+ private volatile boolean mInterrupted;
+
private WorkerWrapper(Builder builder) {
mAppContext = builder.mAppContext;
mWorkSpecId = builder.mWorkSpecId;
@@ -81,6 +83,10 @@
@WorkerThread
@Override
public void run() {
+ if (tryCheckForInterruptionAndNotify()) {
+ return;
+ }
+
mWorkSpec = mWorkSpecDao.getWorkSpec(mWorkSpecId);
if (mWorkSpec == null) {
Log.e(TAG, String.format("Didn't find WorkSpec for id %s", mWorkSpecId));
@@ -129,22 +135,39 @@
// Try to set the work to the running state. Note that this may fail because another thread
// may have modified the DB since we checked last at the top of this function.
if (trySetRunning()) {
+ if (tryCheckForInterruptionAndNotify()) {
+ return;
+ }
+
+ Worker.WorkerResult result = mWorker.doWork();
+
try {
- checkForInterruption();
- Worker.WorkerResult result = mWorker.doWork();
- if (mWorkSpecDao.getState(mWorkSpecId) != CANCELLED) {
- checkForInterruption();
- handleResult(result);
+ mWorkDatabase.beginTransaction();
+ if (!tryCheckForInterruptionAndNotify()) {
+ State state = mWorkSpecDao.getState(mWorkSpecId);
+ if (state == RUNNING) {
+ handleResult(result);
+ } else if (!state.isFinished()) {
+ rescheduleAndNotify();
+ }
+ mWorkDatabase.setTransactionSuccessful();
}
- } catch (InterruptedException e) {
- Log.d(TAG, String.format("Work interrupted for %s", mWorkSpecId));
- rescheduleAndNotify(false);
+ } finally {
+ mWorkDatabase.endTransaction();
}
} else {
notifyIncorrectStatus();
}
}
+ /**
+ * @hide
+ */
+ @RestrictTo(RestrictTo.Scope.LIBRARY_GROUP)
+ public void interrupt() {
+ mInterrupted = true;
+ }
+
private void notifyIncorrectStatus() {
State status = mWorkSpecDao.getState(mWorkSpecId);
if (status == RUNNING) {
@@ -158,10 +181,14 @@
}
}
- private void checkForInterruption() throws InterruptedException {
- if (Thread.currentThread().isInterrupted()) {
- throw new InterruptedException();
+ private boolean tryCheckForInterruptionAndNotify() {
+ if (mInterrupted) {
+ Log.d(TAG, String.format("Work interrupted for %s", mWorkSpecId));
+ State currentState = mWorkSpecDao.getState(mWorkSpecId);
+ notifyListener(currentState == SUCCEEDED, !currentState.isFinished());
+ return true;
}
+ return false;
}
private void notifyListener(final boolean isSuccessful, final boolean needsReschedule) {
@@ -190,7 +217,7 @@
case RETRY: {
Log.d(TAG, String.format("Worker result RETRY for %s", mWorkSpecId));
- rescheduleAndNotify(false /* treating current attempt as a false*/);
+ rescheduleAndNotify();
break;
}
@@ -257,7 +284,7 @@
}
}
- private void rescheduleAndNotify(boolean isSuccessful) {
+ private void rescheduleAndNotify() {
mWorkDatabase.beginTransaction();
try {
mWorkSpecDao.setState(ENQUEUED, mWorkSpecId);
@@ -266,7 +293,7 @@
mWorkDatabase.setTransactionSuccessful();
} finally {
mWorkDatabase.endTransaction();
- notifyListener(isSuccessful, true);
+ notifyListener(false, true);
}
}
diff --git a/work/workmanager/src/main/java/androidx/work/impl/utils/CancelWorkRunnable.java b/work/workmanager/src/main/java/androidx/work/impl/utils/CancelWorkRunnable.java
index 9a2e438..97bea6d 100644
--- a/work/workmanager/src/main/java/androidx/work/impl/utils/CancelWorkRunnable.java
+++ b/work/workmanager/src/main/java/androidx/work/impl/utils/CancelWorkRunnable.java
@@ -47,7 +47,7 @@
recursivelyCancelWorkAndDependents(workManagerImpl.getWorkDatabase(), workSpecId);
Processor processor = workManagerImpl.getProcessor();
- processor.stopWork(workSpecId, true);
+ processor.stopWork(workSpecId);
processor.setCancelled(workSpecId);
for (Scheduler scheduler : workManagerImpl.getSchedulers()) {
diff --git a/work/workmanager/src/main/java/androidx/work/impl/utils/StopWorkRunnable.java b/work/workmanager/src/main/java/androidx/work/impl/utils/StopWorkRunnable.java
index 9a15992..a1ed222 100644
--- a/work/workmanager/src/main/java/androidx/work/impl/utils/StopWorkRunnable.java
+++ b/work/workmanager/src/main/java/androidx/work/impl/utils/StopWorkRunnable.java
@@ -51,9 +51,13 @@
if (workSpecDao.getState(mWorkSpecId) == State.RUNNING) {
workSpecDao.setState(State.ENQUEUED, mWorkSpecId);
}
- boolean isStopped = mWorkManagerImpl.getProcessor().stopWork(mWorkSpecId, true);
- Log.d(TAG, String.format(
- "StopWorkRunnable for %s; Processor.stopWork = %s", mWorkSpecId, isStopped));
+ boolean isStopped = mWorkManagerImpl.getProcessor().stopWork(mWorkSpecId);
+ Log.d(
+ TAG,
+ String.format(
+ "StopWorkRunnable for %s; Processor.stopWork = %s",
+ mWorkSpecId,
+ isStopped));
workDatabase.setTransactionSuccessful();
} finally {
workDatabase.endTransaction();