Sort ref-names before locking them in filterAndLock It is a common practise to avoid locking a set of resources in an unpredictable order, because it would be subject to deadlocks. Example of a possible deadlock is when two replication tasks are trying to fetch two refs but in different order, e.g. refs/heads/branch1 and refs/heads/branch2. By locking the two refs in the same ordering, make sure that you never have a situation whilst one task locks refs/heads/branch1 and the other task locks refs/heads/branch2 and they both are stuck in trying each other branches, when the task one tries to lock refs/heads/branch2 (lock held by task two) and the task two tries to lock refs/heads/branch1 (lock held by task one). Bug: Issue 437805590 Change-Id: I7fdb072e30e2313415c58e8c8dd39dd1a1f9f6e4
diff --git a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/MultisiteReplicationFetchFilter.java b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/MultisiteReplicationFetchFilter.java index ca79bcd..8a4503a 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/MultisiteReplicationFetchFilter.java +++ b/src/main/java/com/googlesource/gerrit/plugins/multisite/validation/MultisiteReplicationFetchFilter.java
@@ -19,6 +19,7 @@ import com.gerritforge.gerrit.globalrefdb.GlobalRefDbLockException; import com.gerritforge.gerrit.globalrefdb.RefDbLockException; import com.gerritforge.gerrit.globalrefdb.validation.SharedRefDatabaseWrapper; +import com.google.common.collect.ImmutableSortedSet; import com.google.common.collect.Sets; import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.Project; @@ -34,6 +35,7 @@ import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.SortedSet; import java.util.stream.Collectors; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Ref; @@ -96,19 +98,23 @@ @Override public Map<String, AutoCloseable> filterAndLock(String projectName, Set<String> fetchRefs) { + // Use a SortedSet to ensure that they are always locked in the same order and therefore + // prevents potential deadlocks where multiple fetch tasks are mutually locking each other + // by locking different refs in different orders. + SortedSet<String> sortedFetchRefs = ImmutableSortedSet.copyOf(fetchRefs); Project.NameKey projectKey = Project.nameKey(projectName); Set<String> filteredRefs = new HashSet<>(); Map<String, AutoCloseable> refLocks = new HashMap<>(); try { - for (String ref : fetchRefs) { + for (String ref : sortedFetchRefs) { refLocks.put(ref, sharedRefDb.lockLocalRef(projectKey, ref)); } - filteredRefs.addAll(filter(projectName, fetchRefs)); + filteredRefs.addAll(filter(projectName, sortedFetchRefs)); } catch (RefDbLockException lockException) { filteredRefs.clear(); throw lockException; } finally { - for (String excludedRef : Sets.difference(fetchRefs, filteredRefs)) { + for (String excludedRef : Sets.difference(sortedFetchRefs, filteredRefs)) { AutoCloseable excludedLock = refLocks.remove(excludedRef); if (excludedLock != null) { try {