Fail an enqueued job if its dependencies already failed.

This was a bug that was most notable during the attachment pre-upload
process: if an attachment failed to upload, the subsequently-enqueued
PushMediaSendJob would still send. This is because the attachment jobs
were enqueued first and failed *before* we enqueued the PushMediaSendJob
as a dependency.

This will use the JobTracker to determine if a dependency already failed
at the time of enqueueing a job like this. This isn't perfect, because
the JobTracker is memory-only and has a limited buffer (currently 1000),
but in practice this should be sufficient for our use cases. I imagine
it'd only fall apart if we somehow  enqueued a dependent job *much*
later, or somehow enqueued it based on a job ID that we persisted on
disk through an app restart. We don't do any of these things, currently,
and probably never should.

Also took the opportunity to patch a case where we weren't failing
dependent jobs when canceling a job, since I was giving the failure
stuff a look-over.
fork-5.53.8
Greyson Parrelli 2021-02-10 12:08:50 -05:00 zatwierdzone przez Cody Henthorne
rodzic b935999548
commit 158f3d898f
2 zmienionych plików z 41 dodań i 8 usunięć

Wyświetl plik

@ -17,11 +17,13 @@ import org.thoughtcrime.securesms.jobmanager.persistence.JobSpec;
import org.thoughtcrime.securesms.jobmanager.persistence.JobStorage;
import org.thoughtcrime.securesms.util.Debouncer;
import org.thoughtcrime.securesms.util.FeatureFlags;
import org.thoughtcrime.securesms.util.SetUtil;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
@ -111,17 +113,29 @@ class JobController {
return;
}
Set<String> dependsOnSet = Stream.of(dependsOn)
.filter(id -> jobStorage.getJobSpec(id) != null)
.collect(Collectors.toSet());
Set<String> allDependsOn = new HashSet<>(dependsOn);
Set<String> aliveDependsOn = Stream.of(dependsOn)
.filter(id -> jobStorage.getJobSpec(id) != null)
.collect(Collectors.toSet());
if (dependsOnQueue != null) {
dependsOnSet.addAll(Stream.of(jobStorage.getJobsInQueue(dependsOnQueue))
.map(JobSpec::getId)
.toList());
List<String> inQueue = Stream.of(jobStorage.getJobsInQueue(dependsOnQueue))
.map(JobSpec::getId)
.toList();
allDependsOn.addAll(inQueue);
aliveDependsOn.addAll(inQueue);
}
FullSpec fullSpec = buildFullSpec(job, dependsOnSet);
if (jobTracker.haveAnyFailed(allDependsOn)) {
Log.w(TAG, "This job depends on a job that failed! Failing this job immediately.");
List<Job> dependents = onFailure(job);
job.onFailure();
Stream.of(dependents).forEach(Job::onFailure);
return;
}
FullSpec fullSpec = buildFullSpec(job, aliveDependsOn);
jobStorage.insertJobs(Collections.singletonList(fullSpec));
scheduleJobs(Collections.singletonList(job));
@ -145,8 +159,9 @@ class JobController {
Log.w(TAG, JobLogger.format(job, "Job failed."));
job.cancel();
List<Job> dependents = onFailure(job);
job.onFailure();
onFailure(job);
Stream.of(dependents).forEach(Job::onFailure);
} else {
Log.w(TAG, "Tried to cancel JOB::" + id + ", but it could not be found.");
}

Wyświetl plik

@ -10,6 +10,8 @@ import org.signal.core.util.concurrent.SignalExecutors;
import org.thoughtcrime.securesms.util.LRUCache;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
@ -66,6 +68,22 @@ public class JobTracker {
});
}
/**
* Returns whether or not any jobs referenced by the IDs in the provided collection have failed.
* Keep in mind that this is not perfect -- our data is only kept in memory, and even then only up
* to a certain limit.
*/
synchronized boolean haveAnyFailed(@NonNull Collection<String> jobIds) {
for (String jobId : jobIds) {
JobInfo jobInfo = jobInfos.get(jobId);
if (jobInfo != null && jobInfo.getJobState() == JobState.FAILURE) {
return true;
}
}
return false;
}
private @NonNull JobInfo getOrCreateJobInfo(@NonNull Job job) {
JobInfo jobInfo = jobInfos.get(job.getId());