From ae4349db632ccefec16647da8b0830c069707335 Mon Sep 17 00:00:00 2001 From: Sven Selberg Date: Mon, 2 Dec 2013 16:54:31 +0100 Subject: [PATCH] Bugfix: Changing Task state breaks comparator in ShowQueue MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 X Change-Id: Iea17046aea1b8c6119cfc663438e17f663e05b22 --- .../gerrit/server/git/TaskInfoFactory.java | 19 +++ .../google/gerrit/server/git/WorkQueue.java | 23 +++- .../gerrit/sshd/commands/ShowQueue.java | 128 ++++++++++++------ 3 files changed, 123 insertions(+), 47 deletions(-) create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/git/TaskInfoFactory.java 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 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 List getTaskInfos(TaskInfoFactory factory) { + List 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 task; if (runnable instanceof ProjectRunnable) { - task = new ProjectTask((ProjectRunnable)runnable, r, this, id); + task = new ProjectTask((ProjectRunnable) runnable, r, this, id); } else { task = new Task(runnable, r, this, id); } @@ -214,6 +226,10 @@ public class WorkQueue { void addAllTo(final List> list) { list.addAll(all.values()); // iterator is thread safe } + + Collection> 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 extends Task 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> pending = workQueue.getTasks(); - Collections.sort(pending, new Comparator>() { - 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 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 getSortedTaskInfoList() { + final List taskInfos = + workQueue.getTaskInfos(new TaskInfoFactory() { + @Override + public QueueTaskInfo getTaskInfo(Task task) { + return new QueueTaskInfo(task); + } + }); + Collections.sort(taskInfos, new Comparator() { + @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); + } + } }