Fix some race conditions in WorkerWrapper.
- check for incorrect status should happen in same
transaction as fetching the workspec
- shortcut code in Schedulers where there are no
eligible workspecs
This was discovered as part of the change that disabled
logging (ag/4407764) causing the Sherlock Holmes
integration app test to fail or become flaky.
Test: Updated and ran tests.
Change-Id: I178cf9051f98f4fd301d5a6d5325aa0a958abb12
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 b389e19..c9a8e64 100644
--- a/work/workmanager/src/androidTest/java/androidx/work/impl/WorkerWrapperTest.java
+++ b/work/workmanager/src/androidTest/java/androidx/work/impl/WorkerWrapperTest.java
@@ -576,16 +576,34 @@
@Test
@SmallTest
public void testScheduler() {
- OneTimeWorkRequest work = new OneTimeWorkRequest.Builder(TestWorker.class).build();
- insertWork(work);
- Scheduler mockScheduler = mock(Scheduler.class);
+ OneTimeWorkRequest prerequisiteWork =
+ new OneTimeWorkRequest.Builder(TestWorker.class).build();
+ OneTimeWorkRequest work = new OneTimeWorkRequest.Builder(TestWorker.class)
+ .setInitialState(BLOCKED).build();
+ Dependency dependency = new Dependency(work.getStringId(), prerequisiteWork.getStringId());
- new WorkerWrapper.Builder(mContext, mConfiguration, mDatabase, work.getStringId())
- .withSchedulers(Collections.singletonList(mockScheduler))
+ mDatabase.beginTransaction();
+ try {
+ insertWork(prerequisiteWork);
+ insertWork(work);
+ mDependencyDao.insertDependency(dependency);
+ mDatabase.setTransactionSuccessful();
+ } finally {
+ mDatabase.endTransaction();
+ }
+
+ new WorkerWrapper.Builder(
+ mContext,
+ mConfiguration,
+ mDatabase,
+ prerequisiteWork.getStringId())
+ .withSchedulers(Collections.singletonList(mMockScheduler))
.build()
.run();
- verify(mockScheduler).schedule();
+ ArgumentCaptor<WorkSpec> captor = ArgumentCaptor.forClass(WorkSpec.class);
+ verify(mMockScheduler).schedule(captor.capture());
+ assertThat(captor.getValue().id, is(work.getStringId()));
}
@Test
diff --git a/work/workmanager/src/main/java/androidx/work/impl/Schedulers.java b/work/workmanager/src/main/java/androidx/work/impl/Schedulers.java
index a31bf46..041f788 100644
--- a/work/workmanager/src/main/java/androidx/work/impl/Schedulers.java
+++ b/work/workmanager/src/main/java/androidx/work/impl/Schedulers.java
@@ -65,43 +65,38 @@
@NonNull Configuration configuration,
@NonNull WorkDatabase workDatabase,
List<Scheduler> schedulers) {
-
- WorkSpecDao workSpecDao = workDatabase.workSpecDao();
- List<WorkSpec> eligibleWorkSpecs =
- workSpecDao.getEligibleWorkForScheduling(
- configuration.getMaxSchedulerLimit());
- scheduleInternal(workDatabase, schedulers, eligibleWorkSpecs);
- }
-
-
-
- private static void scheduleInternal(
- @NonNull WorkDatabase workDatabase,
- List<Scheduler> schedulers,
- List<WorkSpec> workSpecs) {
-
- if (workSpecs == null || schedulers == null) {
+ if (schedulers == null || schedulers.size() == 0) {
return;
}
- long now = System.currentTimeMillis();
WorkSpecDao workSpecDao = workDatabase.workSpecDao();
- // Mark all the WorkSpecs as scheduled.
- // Calls to Scheduler#schedule() could potentially result in more schedules
- // on a separate thread. Therefore, this needs to be done first.
+ List<WorkSpec> eligibleWorkSpecs;
+
workDatabase.beginTransaction();
try {
- for (WorkSpec workSpec : workSpecs) {
- workSpecDao.markWorkSpecScheduled(workSpec.id, now);
+ eligibleWorkSpecs = workSpecDao.getEligibleWorkForScheduling(
+ configuration.getMaxSchedulerLimit());
+ if (eligibleWorkSpecs != null && eligibleWorkSpecs.size() > 0) {
+ long now = System.currentTimeMillis();
+
+ // Mark all the WorkSpecs as scheduled.
+ // Calls to Scheduler#schedule() could potentially result in more schedules
+ // on a separate thread. Therefore, this needs to be done first.
+ for (WorkSpec workSpec : eligibleWorkSpecs) {
+ workSpecDao.markWorkSpecScheduled(workSpec.id, now);
+ }
}
workDatabase.setTransactionSuccessful();
} finally {
workDatabase.endTransaction();
}
- WorkSpec[] eligibleWorkSpecsArray = workSpecs.toArray(new WorkSpec[0]);
- // Delegate to the underlying scheduler.
- for (Scheduler scheduler : schedulers) {
- scheduler.schedule(eligibleWorkSpecsArray);
+
+ if (eligibleWorkSpecs != null && eligibleWorkSpecs.size() > 0) {
+ WorkSpec[] eligibleWorkSpecsArray = eligibleWorkSpecs.toArray(new WorkSpec[0]);
+ // Delegate to the underlying scheduler.
+ for (Scheduler scheduler : schedulers) {
+ scheduler.schedule(eligibleWorkSpecsArray);
+ }
}
}
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 eed0ca4..d510e30 100644
--- a/work/workmanager/src/main/java/androidx/work/impl/WorkerWrapper.java
+++ b/work/workmanager/src/main/java/androidx/work/impl/WorkerWrapper.java
@@ -97,18 +97,28 @@
return;
}
- mWorkSpec = mWorkSpecDao.getWorkSpec(mWorkSpecId);
- if (mWorkSpec == null) {
- Log.e(TAG, String.format("Didn't find WorkSpec for id %s", mWorkSpecId));
- notifyListener(false, false);
- return;
- }
+ mWorkDatabase.beginTransaction();
+ try {
+ mWorkSpec = mWorkSpecDao.getWorkSpec(mWorkSpecId);
+ if (mWorkSpec == null) {
+ Log.e(TAG, String.format("Didn't find WorkSpec for id %s", mWorkSpecId));
+ notifyListener(false, false);
+ return;
+ }
- // Do a quick check to make sure we don't need to bail out in case this work is already
- // running, finished, or is blocked.
- if (mWorkSpec.state != ENQUEUED) {
- notifyIncorrectStatus();
- return;
+ // Do a quick check to make sure we don't need to bail out in case this work is already
+ // running, finished, or is blocked.
+ if (mWorkSpec.state != ENQUEUED) {
+ notifyIncorrectStatus();
+ mWorkDatabase.setTransactionSuccessful();
+ return;
+ }
+
+ // Needed for nested transactions, such as when we're in a dependent work request when
+ // using a SynchronousExecutor.
+ mWorkDatabase.setTransactionSuccessful();
+ } finally {
+ mWorkDatabase.endTransaction();
}
// Merge inputs. This can be potentially expensive code, so this should not be done inside
@@ -283,9 +293,9 @@
if (currentState == ENQUEUED) {
mWorkSpecDao.setState(RUNNING, mWorkSpecId);
mWorkSpecDao.incrementWorkSpecRunAttemptCount(mWorkSpecId);
- mWorkDatabase.setTransactionSuccessful();
setToRunning = true;
}
+ mWorkDatabase.setTransactionSuccessful();
} finally {
mWorkDatabase.endTransaction();
}
