diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/persistence/DependencySpec.java b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/persistence/DependencySpec.java deleted file mode 100644 index b1518b6d56..0000000000 --- a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/persistence/DependencySpec.java +++ /dev/null @@ -1,51 +0,0 @@ -package org.thoughtcrime.securesms.jobmanager.persistence; - -import androidx.annotation.NonNull; - -import java.util.Locale; -import java.util.Objects; - -public final class DependencySpec { - - private final String jobId; - private final String dependsOnJobId; - private final boolean memoryOnly; - - public DependencySpec(@NonNull String jobId, @NonNull String dependsOnJobId, boolean memoryOnly) { - this.jobId = jobId; - this.dependsOnJobId = dependsOnJobId; - this.memoryOnly = memoryOnly; - } - - public @NonNull String getJobId() { - return jobId; - } - - public @NonNull String getDependsOnJobId() { - return dependsOnJobId; - } - - public boolean isMemoryOnly() { - return memoryOnly; - } - - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - DependencySpec that = (DependencySpec) o; - return Objects.equals(jobId, that.jobId) && - Objects.equals(dependsOnJobId, that.dependsOnJobId) && - memoryOnly == that.memoryOnly; - } - - @Override - public int hashCode() { - return Objects.hash(jobId, dependsOnJobId, memoryOnly); - } - - @Override - public @NonNull String toString() { - return String.format(Locale.US, "jobSpecId: JOB::%s | dependsOnJobSpecId: JOB::%s | memoryOnly: %b", jobId, dependsOnJobId, memoryOnly); - } -} diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/persistence/DependencySpec.kt b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/persistence/DependencySpec.kt new file mode 100644 index 0000000000..2285a3c934 --- /dev/null +++ b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/persistence/DependencySpec.kt @@ -0,0 +1,13 @@ +package org.thoughtcrime.securesms.jobmanager.persistence + +import java.util.Locale + +class DependencySpec( + val jobId: String, + val dependsOnJobId: String, + val isMemoryOnly: Boolean +) { + override fun toString(): String { + return String.format(Locale.US, "jobSpecId: JOB::%s | dependsOnJobSpecId: JOB::%s | memoryOnly: %b", jobId, dependsOnJobId, isMemoryOnly) + } +} diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/FastJobStorage.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/FastJobStorage.kt index c235882c5b..3727c4b95d 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/FastJobStorage.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/FastJobStorage.kt @@ -23,7 +23,7 @@ class FastJobStorage(private val jobDatabase: JobDatabase) : JobStorage { jobConstraints += constraintSpec } - for (dependencySpec in jobDatabase.allDependencySpecs) { + for (dependencySpec in jobDatabase.allDependencySpecs.filterNot { it.hasCircularDependency() }) { val jobDependencies: MutableList = dependenciesByJobId.getOrPut(dependencySpec.jobId) { mutableListOf() } jobDependencies += dependencySpec } @@ -275,4 +275,34 @@ class FastJobStorage(private val jobDatabase: JobDatabase) : JobStorage { private fun getJobById(id: String): JobSpec? { return jobs.firstOrNull { it.id == id } } + + /** + * Note that this is currently only checking a specific kind of circular dependency -- ones that are + * created between dependencies and queues. + * + * More specifically, dependencies where one job depends on another job in the same queue that was + * scheduled *after* it. These dependencies will never resolve. Under normal circumstances these + * won't occur, but *could* occur if the user changed their clock (either purposefully or automatically). + * + * Rather than go through and delete them from the database, removing them from memory at load time + * serves the same effect and doesn't require new write methods. This should also be very rare. + */ + private fun DependencySpec.hasCircularDependency(): Boolean { + val job = getJobById(this.jobId) + val dependsOnJob = getJobById(this.dependsOnJobId) + + if (job == null || dependsOnJob == null) { + return false + } + + if (job.queueKey == null || dependsOnJob.queueKey == null) { + return false + } + + if (job.queueKey != dependsOnJob.queueKey) { + return false + } + + return dependsOnJob.createTime > job.createTime + } } diff --git a/app/src/test/java/org/thoughtcrime/securesms/jobs/FastJobStorageTest.java b/app/src/test/java/org/thoughtcrime/securesms/jobs/FastJobStorageTest.java index 1abd5ab6b1..766242a164 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/jobs/FastJobStorageTest.java +++ b/app/src/test/java/org/thoughtcrime/securesms/jobs/FastJobStorageTest.java @@ -39,6 +39,17 @@ public class FastJobStorageTest { DataSet1.assertDependenciesMatch(subject.getAllDependencySpecs()); } + @Test + public void init_removesCircularDependencies() { + FastJobStorage subject = new FastJobStorage(fixedDataDatabase(DataSetCircularDependency.FULL_SPECS)); + + subject.init(); + + DataSetCircularDependency.assertJobsMatch(subject.getAllJobSpecs()); + DataSetCircularDependency.assertConstraintsMatch(subject.getAllConstraintSpecs()); + DataSetCircularDependency.assertDependenciesMatch(subject.getAllDependencySpecs()); + } + @Test public void insertJobs_writesToDatabase() { JobDatabase database = noopDatabase(); @@ -617,21 +628,21 @@ public class FastJobStorageTest { static void assertJobsMatch(@NonNull List jobs) { assertEquals(jobs.size(), 3); - assertTrue(jobs.contains(DataSet1.JOB_1)); - assertTrue(jobs.contains(DataSet1.JOB_1)); - assertTrue(jobs.contains(DataSet1.JOB_3)); + assertTrue(jobs.contains(JOB_1)); + assertTrue(jobs.contains(JOB_2)); + assertTrue(jobs.contains(JOB_3)); } static void assertConstraintsMatch(@NonNull List constraints) { assertEquals(constraints.size(), 2); - assertTrue(constraints.contains(DataSet1.CONSTRAINT_1)); - assertTrue(constraints.contains(DataSet1.CONSTRAINT_2)); + assertTrue(constraints.contains(CONSTRAINT_1)); + assertTrue(constraints.contains(CONSTRAINT_2)); } static void assertDependenciesMatch(@NonNull List dependencies) { assertEquals(dependencies.size(), 2); - assertTrue(dependencies.contains(DataSet1.DEPENDENCY_2)); - assertTrue(dependencies.contains(DataSet1.DEPENDENCY_3)); + assertTrue(dependencies.contains(DEPENDENCY_2)); + assertTrue(dependencies.contains(DEPENDENCY_3)); } } @@ -641,4 +652,33 @@ public class FastJobStorageTest { static final FullSpec FULL_SPEC_1 = new FullSpec(JOB_1, Collections.singletonList(CONSTRAINT_1), Collections.emptyList()); static final List FULL_SPECS = Collections.singletonList(FULL_SPEC_1); } + + private static final class DataSetCircularDependency { + static final JobSpec JOB_1 = new JobSpec("id1", "f1", "q1", 1, 2, 3, 4, 5, null, null, false, false); + static final JobSpec JOB_2 = new JobSpec("id2", "f2", "q1", 2, 2, 3, 4, 5, null, null, false, false); + static final JobSpec JOB_3 = new JobSpec("id3", "f3", "q3", 3, 2, 3, 4, 5, null, null, false, false); + static final DependencySpec DEPENDENCY_1 = new DependencySpec("id1", "id2", false); + static final DependencySpec DEPENDENCY_3 = new DependencySpec("id3", "id2", false); + static final FullSpec FULL_SPEC_1 = new FullSpec(JOB_1, Collections.emptyList(), Collections.singletonList(DEPENDENCY_1)); + static final FullSpec FULL_SPEC_2 = new FullSpec(JOB_2, Collections.emptyList(), Collections.emptyList()); + static final FullSpec FULL_SPEC_3 = new FullSpec(JOB_3, Collections.emptyList(), Collections.singletonList(DEPENDENCY_3)); + static final List FULL_SPECS = Arrays.asList(FULL_SPEC_1, FULL_SPEC_2, FULL_SPEC_3); + + static void assertJobsMatch(@NonNull List jobs) { + assertEquals(jobs.size(), 3); + assertTrue(jobs.contains(JOB_1)); + assertTrue(jobs.contains(JOB_2)); + assertTrue(jobs.contains(JOB_3)); + } + + static void assertConstraintsMatch(@NonNull List constraints) { + assertEquals(constraints.size(), 0); + } + + static void assertDependenciesMatch(@NonNull List dependencies) { + assertEquals(dependencies.size(), 1); + assertFalse(dependencies.contains(DEPENDENCY_1)); + assertTrue(dependencies.contains(DEPENDENCY_3)); + } + } }