Prevent multiple instances of the same job running concurrently.

There are rare corner cases where a Job could be preempted by the
JobScheduler and then be rescheduled before the preempted job finished
running. This could create weird race conditions with bad consequences.

To fix this, we create a fun locking system that prevents two jobs with
the same UUID from running at the same time.
This commit is contained in:
Greyson Parrelli 2018-10-28 22:13:51 -07:00
parent f15fb904bf
commit b7b9554364
2 changed files with 121 additions and 4 deletions

View File

@ -14,6 +14,8 @@ import org.thoughtcrime.securesms.logging.Log;
import org.thoughtcrime.securesms.service.GenericForegroundService; import org.thoughtcrime.securesms.service.GenericForegroundService;
import java.io.Serializable; import java.io.Serializable;
import java.util.HashMap;
import java.util.Map;
import java.util.UUID; import java.util.UUID;
import androidx.work.Data; import androidx.work.Data;
@ -26,6 +28,8 @@ public abstract class Job extends Worker implements Serializable {
private static final String TAG = Job.class.getSimpleName(); private static final String TAG = Job.class.getSimpleName();
private static final WorkLockManager WORK_LOCK_MANAGER = new WorkLockManager();
static final String KEY_RETRY_COUNT = "Job_retry_count"; static final String KEY_RETRY_COUNT = "Job_retry_count";
static final String KEY_RETRY_UNTIL = "Job_retry_until"; static final String KEY_RETRY_UNTIL = "Job_retry_until";
static final String KEY_SUBMIT_TIME = "Job_submit_time"; static final String KEY_SUBMIT_TIME = "Job_submit_time";
@ -46,12 +50,28 @@ public abstract class Job extends Worker implements Serializable {
this.parameters = parameters; this.parameters = parameters;
} }
@NonNull
@Override @Override
public Result doWork() { public @NonNull Result doWork() {
log("doWork()" + logSuffix());
try (WorkLockManager.WorkLock workLock = WORK_LOCK_MANAGER.acquire(getId())) {
Result result = workLock.getResult();
if (result == null) {
result = doWorkInternal();
workLock.setResult(result);
} else {
log("Using result from preempted run (" + result + ")." + logSuffix());
}
return result;
}
}
private @NonNull Result doWorkInternal() {
Data data = getInputData(); Data data = getInputData();
log("doWork()" + logSuffix()); log("doWorkInternal()" + logSuffix());
ApplicationContext.getInstance(getApplicationContext()).injectDependencies(this); ApplicationContext.getInstance(getApplicationContext()).injectDependencies(this);
@ -104,6 +124,8 @@ public abstract class Job extends Worker implements Serializable {
if (cancelled) { if (cancelled) {
warn("onStopped() with cancellation signal." + logSuffix()); warn("onStopped() with cancellation signal." + logSuffix());
onCanceled(); onCanceled();
} else {
log("onStopped()" + logSuffix());
} }
} }
@ -242,6 +264,7 @@ public abstract class Job extends Worker implements Serializable {
private String logSuffix() { private String logSuffix() {
long timeSinceSubmission = System.currentTimeMillis() - getInputData().getLong(KEY_SUBMIT_TIME, 0); long timeSinceSubmission = System.currentTimeMillis() - getInputData().getLong(KEY_SUBMIT_TIME, 0);
return " (Time since submission: " + timeSinceSubmission + " ms, Run attempt: " + getRunAttemptCount() + ")"; return " (Time since submission: " + timeSinceSubmission + " ms, Run attempt: " + getRunAttemptCount() + ", isStopped: " + isStopped() + ")";
} }
} }

View File

@ -0,0 +1,94 @@
package org.thoughtcrime.securesms.jobmanager;
import android.support.annotation.NonNull;
import android.support.annotation.Nullable;
import java.io.Closeable;
import java.util.HashMap;
import java.util.Map;
import java.util.UUID;
import java.util.concurrent.Semaphore;
import androidx.work.Worker;
class WorkLockManager {
private final Map<UUID, WorkLock> locks = new HashMap<>();
WorkLock acquire(@NonNull UUID uuid) {
WorkLock workLock;
synchronized (this) {
workLock = locks.get(uuid);
if (workLock == null) {
workLock = new WorkLock(uuid);
locks.put(uuid, workLock);
}
workLock.increment();
}
workLock.getLock().acquireUninterruptibly();
return workLock;
}
private void release(@NonNull UUID uuid) {
WorkLock lock;
synchronized (this) {
lock = locks.get(uuid);
if (lock == null) {
throw new IllegalStateException("Released a lock that was already removed from use.");
}
if (lock.decrementAndGet() == 0) {
locks.remove(uuid);
}
}
lock.getLock().release();
}
class WorkLock implements Closeable {
private final Semaphore lock;
private final UUID uuid;
private Worker.Result result;
private int count;
private WorkLock(@NonNull UUID uuid) {
this.uuid = uuid;
this.lock = new Semaphore(1);
}
private void increment() {
count++;
}
private int decrementAndGet() {
count--;
return count;
}
private @NonNull Semaphore getLock() {
return lock;
}
void setResult(@NonNull Worker.Result result) {
this.result = result;
}
@Nullable Worker.Result getResult() {
return result;
}
@Override
public void close() {
WorkLockManager.this.release(uuid);
}
}
}