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

3 files changed, 123 insertions, 47 deletions

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 0000000000..ac0772951e
--- /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 bb11e6252e..66f01f611b 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 org.slf4j.LoggerFactory;
 
 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 @@ public class WorkQueue {
     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 @@ public class WorkQueue {
         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 @@ public class WorkQueue {
     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 @@ public class WorkQueue {
     }
   }
 
-  /** 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 fee5275727..7f01aada6f 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 @@ package com.google.gerrit.sshd.commands;
 
 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 @@ final class ShowQueue extends SshCommand {
 
   @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 class ShowQueue extends SshCommand {
     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 @@ final class ShowQueue extends SshCommand {
       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 @@ final class ShowQueue extends SshCommand {
 
       // 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 @@ final class ShowQueue extends SshCommand {
           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 @@ final class ShowQueue extends SshCommand {
     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 @@ final class ShowQueue extends SshCommand {
     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 @@ final class ShowQueue extends SshCommand {
         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);
+    }
+  }
 }