Skip to content

Commit

Permalink
Fail an enqueued job if its dependencies already failed.
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
greyson-signal authored and cody-signal committed Feb 10, 2021
1 parent b935999 commit 158f3d8
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 8 deletions.
Expand Up @@ -17,11 +17,13 @@
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;
Expand Down Expand Up @@ -111,17 +113,29 @@ synchronized void submitJobWithExistingDependencies(@NonNull Job job, @NonNull C
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);
}

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, dependsOnSet);
FullSpec fullSpec = buildFullSpec(job, aliveDependsOn);
jobStorage.insertJobs(Collections.singletonList(fullSpec));

scheduleJobs(Collections.singletonList(job));
Expand All @@ -145,8 +159,9 @@ synchronized void cancelJob(@NonNull String id) {
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.");
}
Expand Down
Expand Up @@ -10,6 +10,8 @@
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;
Expand Down Expand Up @@ -66,6 +68,22 @@ synchronized void onStateChange(@NonNull Job job, @NonNull JobState state) {
});
}

/**
* 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());

Expand Down

0 comments on commit 158f3d8

Please sign in to comment.