Bugfix: Changing Task state breaks comparator in ShowQueue
This can happens if you have a long queue and the state of
a task (DONE, CANCELLED, RUNNING, READY, SLEEPING, OTHER)
changes while the sorting is ongoing. The reason this
generates an error is because the Task State defines
the tasks’ place in the queue.
If Task state changes while the sorting of the queue is
ongoing the Comparator violates its contract of:
X<Y, Y<Z => X<Z
and throws:
IllegalArgumentException: Comparison mehtod violates its
general contract!
Fixed this bug by saving a snapshot of the state and delay
of the tasks in a wrapper.
* Introduced interface TaskInfo that is implemented by
QueueTaskInfo
* Added getTaskInfos method in WorkQueue decoupling it from
ShowQueue implementation by Interface and factory.
Signed-off-by: Gustaf Lundh <gustaf.lundh@sonymobile.com>
Change-Id: Iea17046aea1b8c6119cfc663438e17f663e05b22
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/TaskInfoFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/TaskInfoFactory.java
new file mode 100644
index 0000000..ac07729
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/TaskInfoFactory.java
@@ -0,0 +1,19 @@
+// Copyright (C) 2013 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.git;
+
+public interface TaskInfoFactory<T> {
+ T getTaskInfo(WorkQueue.Task<?> task);
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java
index bb11e62..66f01f6 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.git;
+import com.google.common.collect.Lists;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.lifecycle.LifecycleModule;
import com.google.gerrit.reviewdb.client.Project.NameKey;
@@ -26,6 +27,7 @@
import java.lang.Thread.UncaughtExceptionHandler;
import java.util.ArrayList;
+import java.util.Collection;
import java.util.List;
import java.util.concurrent.Callable;
import java.util.concurrent.ConcurrentHashMap;
@@ -115,6 +117,16 @@
return r;
}
+ public <T> List<T> getTaskInfos(TaskInfoFactory<T> factory) {
+ List<T> taskInfos = Lists.newArrayList();
+ for (Executor exe : queues) {
+ for (Task<?> task : exe.getTasks()) {
+ taskInfos.add(factory.getTaskInfo(task));
+ }
+ }
+ return taskInfos;
+ }
+
/** Locate a task by its unique id, null if no task matches. */
public Task<?> getTask(final int id) {
Task<?> result = null;
@@ -186,7 +198,7 @@
Task<V> task;
if (runnable instanceof ProjectRunnable) {
- task = new ProjectTask<V>((ProjectRunnable)runnable, r, this, id);
+ task = new ProjectTask<V>((ProjectRunnable) runnable, r, this, id);
} else {
task = new Task<V>(runnable, r, this, id);
}
@@ -214,6 +226,10 @@
void addAllTo(final List<Task<?>> list) {
list.addAll(all.values()); // iterator is thread safe
}
+
+ Collection<Task<?>> getTasks() {
+ return all.values();
+ }
}
/** Runnable needing to know it was canceled. */
@@ -351,8 +367,9 @@
}
}
- /** Same as Task class, but with a reference to ProjectRunnable, used to retrieve
- * the project name from the operation queued
+ /**
+ * Same as Task class, but with a reference to ProjectRunnable, used to
+ * retrieve the project name from the operation queued
**/
public static class ProjectTask<V> extends Task<V> implements ProjectRunnable {
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ShowQueue.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ShowQueue.java
index fee5275..7f01aad 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ShowQueue.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ShowQueue.java
@@ -16,6 +16,7 @@
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.git.TaskInfoFactory;
import com.google.gerrit.server.git.WorkQueue;
import com.google.gerrit.server.git.WorkQueue.ProjectTask;
import com.google.gerrit.server.git.WorkQueue.Task;
@@ -72,31 +73,10 @@
@Override
protected void run() {
- final List<Task<?>> pending = workQueue.getTasks();
- Collections.sort(pending, new Comparator<Task<?>>() {
- public int compare(Task<?> a, Task<?> b) {
- final Task.State aState = a.getState();
- final Task.State bState = b.getState();
-
- if (aState != bState) {
- return aState.ordinal() - bState.ordinal();
- }
-
- final long aDelay = a.getDelay(TimeUnit.MILLISECONDS);
- final long bDelay = b.getDelay(TimeUnit.MILLISECONDS);
-
- if (aDelay < bDelay) {
- return -1;
- } else if (aDelay > bDelay) {
- return 1;
- }
- return format(a).compareTo(format(b));
- }
- });
-
taskNameWidth = wide ? Integer.MAX_VALUE : columns - 8 - 12 - 8 - 4;
+ final List<QueueTaskInfo> pending = getSortedTaskInfoList();
- stdout.print(String.format("%-8s %-12s %-8s %s\n", //
+ stdout.print(String.format("%-8s %-12s %-8s %s\n",
"Task", "State", "", "Command"));
stdout.print("----------------------------------------------"
+ "--------------------------------\n");
@@ -105,9 +85,9 @@
final long now = System.currentTimeMillis();
final boolean viewAll = currentUser.getCapabilities().canViewQueue();
- for (final Task<?> task : pending) {
- final long delay = task.getDelay(TimeUnit.MILLISECONDS);
- final Task.State state = task.getState();
+ for (final QueueTaskInfo taskInfo : pending) {
+ final long delay = taskInfo.delayMillis;
+ final Task.State state = taskInfo.state;
final String start;
switch (state) {
@@ -131,11 +111,9 @@
String remoteName = null;
if (!viewAll) {
- if (task instanceof ProjectTask<?>) {
- projectName = ((ProjectTask<?>)task).getProjectNameKey();
- remoteName = ((ProjectTask<?>)task).getRemoteName();
- hasCustomizedPrint = ((ProjectTask<?>)task).hasCustomizedPrint();
- }
+ projectName = taskInfo.getProjectNameKey();
+ remoteName = taskInfo.getRemoteName();
+ hasCustomizedPrint = taskInfo.hasCustomizedPrint();
ProjectState e = null;
if (projectName != null) {
@@ -151,8 +129,10 @@
// Shows information about tasks depending on the user rights
if (viewAll || (!hasCustomizedPrint && regularUserCanSee)) {
- stdout.print(String.format("%8s %-12s %-8s %s\n", //
- id(task.getTaskId()), start, "", format(task)));
+ stdout.print(String.format(
+ "%8s %-12s %-8s %s\n",
+ id(taskInfo.getTaskId()), start, "",
+ taskInfo.getTaskString(taskNameWidth)));
} else if (regularUserCanSee) {
if (remoteName == null) {
remoteName = projectName.get();
@@ -160,8 +140,8 @@
remoteName = remoteName + "/" + projectName;
}
- stdout.print(String.format("%8s %-12s %-8s %s\n", //
- id(task.getTaskId()), start, "", remoteName));
+ stdout.print(String.format("%8s %-12s %-8s %s\n",
+ id(taskInfo.getTaskId()), start, "", remoteName));
}
}
stdout.print("----------------------------------------------"
@@ -174,6 +154,33 @@
stdout.print(" " + numberOfPendingTasks + " tasks\n");
}
+ private List<QueueTaskInfo> getSortedTaskInfoList() {
+ final List<QueueTaskInfo> taskInfos =
+ workQueue.getTaskInfos(new TaskInfoFactory<QueueTaskInfo>() {
+ @Override
+ public QueueTaskInfo getTaskInfo(Task<?> task) {
+ return new QueueTaskInfo(task);
+ }
+ });
+ Collections.sort(taskInfos, new Comparator<QueueTaskInfo>() {
+ @Override
+ public int compare(QueueTaskInfo a, QueueTaskInfo b) {
+ if (a.state != b.state) {
+ return a.state.ordinal() - b.state.ordinal();
+ }
+
+ int cmp = Long.signum(a.delayMillis - b.delayMillis);
+ if (cmp != 0) {
+ return cmp;
+ }
+
+ return a.getTaskString(taskNameWidth)
+ .compareTo(b.getTaskString(taskNameWidth));
+ }
+ });
+ return taskInfos;
+ }
+
private static String id(final int id) {
return IdGenerator.format(id);
}
@@ -186,15 +193,6 @@
return new SimpleDateFormat("MMM-dd HH:mm").format(when);
}
- private String format(final Task<?> task) {
- String s = task.toString();
- if (s.length() < taskNameWidth) {
- return s;
- } else {
- return s.substring(0, taskNameWidth);
- }
- }
-
private static String format(final Task.State state) {
switch (state) {
case DONE:
@@ -211,4 +209,46 @@
return state.toString();
}
}
+
+ private static class QueueTaskInfo {
+ private final long delayMillis;
+ private final Task.State state;
+ private final Task<?> task;
+
+ QueueTaskInfo(Task<?> task) {
+ this.task = task;
+ this.delayMillis = task.getDelay(TimeUnit.MILLISECONDS);
+ this.state = task.getState();
+ }
+
+ String getRemoteName() {
+ if (task instanceof ProjectTask) {
+ return ((ProjectTask<?>) task).getRemoteName();
+ }
+ return null;
+ }
+
+ Project.NameKey getProjectNameKey() {
+ if (task instanceof ProjectTask<?>) {
+ return ((ProjectTask<?>) task).getProjectNameKey();
+ }
+ return null;
+ }
+
+ boolean hasCustomizedPrint() {
+ if (task instanceof ProjectTask<?>) {
+ return ((ProjectTask<?>) task).hasCustomizedPrint();
+ }
+ return false;
+ }
+
+ int getTaskId() {
+ return task.getTaskId();
+ }
+
+ String getTaskString(int maxLength) {
+ String s = task.toString();
+ return s.length() < maxLength ? s : s.substring(0, maxLength);
+ }
+ }
}
