WorkQueue: Skip `isReadyToStart` invocation for CANCELLED tasks
In normal circumstances `isReadyToStart` is invoked before `onStop` for
task listeners. However, this contract is not upheld in case a task is
cancelled while it's being un-parked and `isReadyToStart` and `onStop`
can be invoked out of order. This becomes a problem if the listener
implementation for `isReadyToStart` is non-idempotent (it maintains a
state) and relies on `onStop` for cleanup and resetting the state.
This change makes sure that these two methods are never invoked out of
order. The following example explains the logic in detail:
Let's assume there are a couple of tasks, such that task1 is RUNNING
and task2 is PARKED. Now there are a couple of ways in which task2 can
be un-parked:
Task Complete Path:
When task1 completes, task2 is polled from the parked queue to check if
it's ready to start (using `isReadyToStart` invocation on listeners). If
yes, then task2 is un-parked using latch countdown.
Task Cancel Path:
If task2 is cancelled and it is present in parked queue, latch countdown
is performed to un-park it and then `onStop` is invoked on the listeners
to take care of clean-up.
The problem arises when the above two operations occur simultaneously,
creating a race between `isReadyToStart` and `onStop` invocations for
the task, which in turn can lead to out of order invocation of these
methods.
This race however, can occur only if task2 is cancelled but it gets
polled and isReadyToStart is invoked on 'Task Complete Path' before it
is removed from parked queue on 'Task Cancel Path', after the invocation
of ParkedTask::cancel.
The other two cases will not result in race as explained below:
Case 1:
If task2 is polled before ParkedTask::cancel is invoked, `onStop` will
not be invoked on 'Task Cancel Path'
Case 2:
If task2 is removed from parked queue on 'Task Cancel Path' before it is
polled, `isReadyToStart` will not be invoked on 'Task Complete Path'
Therefore, if `isReadyToStart` invocation is skipped for cancelled tasks
the race will never occur.
Change-Id: Ic1b8cc36818062339ef55e15cfa8a23ba7a598bb
Release-Notes: skip
diff --git a/java/com/google/gerrit/server/git/WorkQueue.java b/java/com/google/gerrit/server/git/WorkQueue.java
index 97b5abe..c663e0f 100644
--- a/java/com/google/gerrit/server/git/WorkQueue.java
+++ b/java/com/google/gerrit/server/git/WorkQueue.java
@@ -41,7 +41,6 @@
import java.util.Collection;
import java.util.HashSet;
import java.util.List;
-import java.util.Optional;
import java.util.Set;
import java.util.concurrent.Callable;
import java.util.concurrent.ConcurrentHashMap;
@@ -616,10 +615,7 @@
}
void cancelIfParked(Task<?> task) {
- Optional<ParkedTask> parkedTask = parked.stream().filter(p -> p.isEqualTo(task)).findFirst();
- if (parkedTask.isPresent()) {
- parkedTask.get().cancel();
- }
+ parked.stream().filter(p -> p.isEqualTo(task)).findFirst().ifPresent(ParkedTask::cancel);
}
Task<?> getTask(int id) {
@@ -691,17 +687,19 @@
}
public void updateParked() {
- ParkedTask ready = parked.poll();
- if (ready == null) {
- return;
- }
List<ParkedTask> notReady = new ArrayList<>();
- while (ready != null && !isReadyToStart(ready.task)) {
- // Do not add a cancelled task back into the parked queue
- if (Task.State.PARKED.equals(ready.task.getState())) {
+ ParkedTask ready;
+
+ while ((ready = parked.poll()) != null) {
+ if (Task.State.CANCELLED.equals(ready.task.getState())) {
+ ready.cancel(); // In case a cancelled task is polled before cleanup
+ } else if (isReadyToStart(ready.task)) {
+ break;
+ } else if (Task.State.CANCELLED.equals(ready.task.getState())) {
+ ready.cancel(); // In case the task is cancelled while evaluating isReadyToStart
+ } else {
notReady.add(ready);
}
- ready = parked.poll();
}
parked.addAll(notReady);
