From fc76a027debbe15375da7b9896919143c310d1e0 Mon Sep 17 00:00:00 2001 From: Andrew Steinborn Date: Thu, 13 Dec 2018 04:06:43 -0500 Subject: [PATCH] Rewrite dependency plugin resolving. Velocity now resolves plugin dependencies using a depth-first search (DFS), instead of using Kahn's algorithm like before. This properly handles duplicate indirect dependencies. Thanks to @creeper123123321 for noticing this issue! --- .../plugin/util/PluginDependencyUtils.java | 86 +++++-------------- .../util/PluginDependencyUtilsTest.java | 17 +++- 2 files changed, 37 insertions(+), 66 deletions(-) diff --git a/proxy/src/main/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtils.java b/proxy/src/main/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtils.java index 2ba635c19..93c5b341d 100644 --- a/proxy/src/main/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtils.java +++ b/proxy/src/main/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtils.java @@ -1,20 +1,15 @@ package com.velocitypowered.proxy.plugin.util; -import com.google.common.collect.ImmutableSet; import com.google.common.collect.Maps; -import com.google.common.graph.EndpointPair; import com.google.common.graph.Graph; import com.google.common.graph.GraphBuilder; import com.google.common.graph.MutableGraph; import com.velocitypowered.api.plugin.PluginDescription; import com.velocitypowered.api.plugin.meta.PluginDependency; -import java.util.ArrayDeque; import java.util.ArrayList; -import java.util.HashSet; +import java.util.HashMap; import java.util.List; import java.util.Map; -import java.util.Queue; -import java.util.Set; public class PluginDependencyUtils { @@ -24,7 +19,7 @@ public class PluginDependencyUtils { /** * Attempts to topographically sort all plugins for the proxy to load by dependencies using - * Kahn's algorithm. + * a depth-first search. * * @param candidates the plugins to sort * @return the sorted list of plugins @@ -49,72 +44,37 @@ public class PluginDependencyUtils { } } - // Find nodes that have no edges - Queue noEdges = getNoDependencyCandidates(graph); - - // Actually run Kahn's algorithm List sorted = new ArrayList<>(); - while (!noEdges.isEmpty()) { - PluginDescription candidate = noEdges.remove(); - sorted.add(candidate); + Map marks = new HashMap<>(); - for (PluginDescription node : ImmutableSet.copyOf(graph.adjacentNodes(candidate))) { - graph.removeEdge(node, candidate); - - if (graph.adjacentNodes(node).isEmpty()) { - noEdges.add(node); - } - } - } - - if (!graph.edges().isEmpty()) { - throw new IllegalStateException( - "Plugin circular dependency found: " + createLoopInformation(graph)); + for (PluginDescription node : graph.nodes()) { + visitNode(graph, node, marks, sorted); } return sorted; } - private static Queue getNoDependencyCandidates(Graph graph) { - Queue found = new ArrayDeque<>(); - - for (PluginDescription node : graph.nodes()) { - if (graph.outDegree(node) == 0) { - found.add(node); - } + private static void visitNode(Graph dependencyGraph, PluginDescription node, + Map marks, List sorted) { + Mark mark = marks.getOrDefault(node, Mark.NOT_VISITED); + if (mark == Mark.PERMANENT) { + return; + } else if (mark == Mark.TEMPORARY) { + throw new IllegalStateException("Improper plugin dependency graph"); } - return found; + marks.put(node, Mark.TEMPORARY); + for (PluginDescription edge : dependencyGraph.successors(node)) { + visitNode(dependencyGraph, edge, marks, sorted); + } + + marks.put(node, Mark.PERMANENT); + sorted.add(node); } - private static String createLoopInformation(Graph graph) { - StringBuilder repr = new StringBuilder("{"); - for (EndpointPair edge : graph.edges()) { - repr.append(edge.target().getId()).append(": ["); - repr.append(dependencyLoopInfo(graph, edge.target(), new HashSet<>())).append("], "); - } - repr.setLength(repr.length() - 2); - repr.append("}"); - return repr.toString(); - } - - private static String dependencyLoopInfo(Graph graph, - PluginDescription dependency, Set seen) { - StringBuilder repr = new StringBuilder(); - for (PluginDescription pd : graph.adjacentNodes(dependency)) { - if (seen.add(pd)) { - repr.append(pd.getId()).append(": [").append(dependencyLoopInfo(graph, dependency, seen)) - .append("], "); - } else { - repr.append(pd.getId()).append(", "); - } - } - - if (repr.length() != 0) { - repr.setLength(repr.length() - 2); - return repr.toString(); - } else { - return ""; - } + private enum Mark { + NOT_VISITED, + TEMPORARY, + PERMANENT } } diff --git a/proxy/src/test/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtilsTest.java b/proxy/src/test/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtilsTest.java index 304bf4908..a5c5080ee 100644 --- a/proxy/src/test/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtilsTest.java +++ b/proxy/src/test/java/com/velocitypowered/proxy/plugin/util/PluginDependencyUtilsTest.java @@ -8,6 +8,7 @@ import com.velocitypowered.api.plugin.PluginDescription; import com.velocitypowered.api.plugin.meta.PluginDependency; import com.velocitypowered.proxy.plugin.loader.VelocityPluginDescription; import java.util.ArrayList; +import java.util.Comparator; import java.util.List; import org.junit.jupiter.api.Test; @@ -38,6 +39,13 @@ class PluginDependencyUtilsTest { new PluginDependency("example2", "", false) ), null ); + private static final PluginDescription TEST_WITH_DUPLICATE_DEPEND = new VelocityPluginDescription( + "dup-depend", "tuxed", "0.1", null, null, ImmutableList.of(), + ImmutableList.of( + new PluginDependency("multi-depend", "", false), + new PluginDependency("example2", "", false) + ), null + ); private static final PluginDescription CIRCULAR_DEPENDENCY_1 = new VelocityPluginDescription( "circle", "tuxed", "0.1", null, null, ImmutableList.of(), @@ -55,12 +63,13 @@ class PluginDependencyUtilsTest { // Note: Kahn's algorithm is non-unique in its return result, although the topological sort will have the correct // order. private static final List EXPECTED = ImmutableList.of( + NEVER_DEPENDED, NO_DEPENDENCY_1_EXAMPLE, NO_DEPENDENCY_2_EXAMPLE, - NEVER_DEPENDED, + MULTI_DEPENDENCY, + TEST_WITH_DUPLICATE_DEPEND, SOFT_DEPENDENCY_DOES_NOT_EXIST, - SOFT_DEPENDENCY_EXISTS, - MULTI_DEPENDENCY + SOFT_DEPENDENCY_EXISTS ); @Test @@ -72,6 +81,8 @@ class PluginDependencyUtilsTest { descriptionList.add(SOFT_DEPENDENCY_DOES_NOT_EXIST); descriptionList.add(SOFT_DEPENDENCY_EXISTS); descriptionList.add(MULTI_DEPENDENCY); + descriptionList.add(TEST_WITH_DUPLICATE_DEPEND); + descriptionList.sort(Comparator.comparing(PluginDescription::getId)); assertEquals(EXPECTED, PluginDependencyUtils.sortCandidates(descriptionList)); }