Evict ref from the ref cache even when ref update fails
If for some reason there is a miss-match between a persisted ref value
and the cached value of that ref, it is likely to result in a ref-update
failure. There are many places in Gerrit where code is retried on
ref-update failures with the hopes that the failure can be recovered
from. To make this recovery possible, it is essential to ensure that
the outdated cached ref value no longer be used, and evicting the cached
value even on failures makes this possible.
Some reasons where cached ref values could be outdated are: 3rd party
actors modifying the repos outside of Gerrit, multi-primary setups.
For example, consider a multi-primary Gerrit setup with 2 primaries
pointing to the repositories on NFS. When one primary receives a review
command and at the same time another primary updates the same change
with a new patchset and succeeds, then the review command would fail
with Lock failure. In this case, review command would retry the
operation, but it fails again because the cached ref value is still
outdated.
In order for a retry operation to succeed, we need to invalidate the
ref from the cache when ref update fails.
Change-Id: I724d5ba3d756e6a22a0b8c64be7ebd4f82d69d14
diff --git a/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/BatchRefUpdateWithCacheUpdate.java b/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/BatchRefUpdateWithCacheUpdate.java
index c3b8537..cf1340f 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/BatchRefUpdateWithCacheUpdate.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/BatchRefUpdateWithCacheUpdate.java
@@ -160,14 +160,20 @@
@Override
public void execute(RevWalk walk, ProgressMonitor monitor, List<String> options)
throws IOException {
- delegate.execute(walk, monitor, options);
- evictCache();
+ try {
+ delegate.execute(walk, monitor, options);
+ } finally {
+ evictCache();
+ }
}
@Override
public void execute(RevWalk walk, ProgressMonitor monitor) throws IOException {
- delegate.execute(walk, monitor);
- evictCache();
+ try {
+ delegate.execute(walk, monitor);
+ } finally {
+ evictCache();
+ }
}
private void evictCache() {
@@ -175,9 +181,7 @@
.getCommands()
.forEach(
cmd -> {
- if (cmd.getResult() == ReceiveCommand.Result.OK) {
- refsCache.evict(repo.getProjectName(), cmd.getRefName());
- }
+ refsCache.evict(repo.getProjectName(), cmd.getRefName());
});
}
}
diff --git a/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/RefRenameWithCacheUpdate.java b/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/RefRenameWithCacheUpdate.java
index f6debce..ed99ff3 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/RefRenameWithCacheUpdate.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/RefRenameWithCacheUpdate.java
@@ -17,7 +17,6 @@
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
-import java.util.EnumSet;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.RefRename;
import org.eclipse.jgit.lib.RefUpdate;
@@ -33,8 +32,6 @@
}
private static final String NOT_SUPPORTED_MSG = "Should never be called";
- private static final EnumSet<Result> SUCCESSFUL_RENAMES =
- EnumSet.of(Result.NEW, Result.FORCED, Result.FAST_FORWARD, Result.RENAMED);
private final RefByNameCacheWrapper refsCache;
private final CachedRefRepository repo;
@@ -92,11 +89,11 @@
@Override
public Result rename() throws IOException {
- Result r = delegate.rename();
- if (SUCCESSFUL_RENAMES.contains(r)) {
+ try {
+ return delegate.rename();
+ } finally {
refsCache.evict(repo.getProjectName(), src.getName());
}
- return r;
}
@Override
diff --git a/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/RefUpdateWithCacheUpdate.java b/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/RefUpdateWithCacheUpdate.java
index 76579ce..99e3dc7 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/RefUpdateWithCacheUpdate.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/cachedrefdb/RefUpdateWithCacheUpdate.java
@@ -3,7 +3,6 @@
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
-import java.util.EnumSet;
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
@@ -21,8 +20,6 @@
}
private static final String NOT_SUPPORTED_MSG = "Should never be called";
- private static final EnumSet<Result> SUCCESSFUL_UPDATES =
- EnumSet.of(Result.NEW, Result.FORCED, Result.FAST_FORWARD, Result.RENAMED);
private final RefByNameCacheWrapper refsCache;
private final RefDatabase refDb;
@@ -139,32 +136,56 @@
@Override
public Result forceUpdate() throws IOException {
- return evictCache(delegate.forceUpdate());
+ try {
+ return delegate.forceUpdate();
+ } finally {
+ evictCache();
+ }
}
@Override
public Result update() throws IOException {
- return evictCache(delegate.update());
+ try {
+ return delegate.update();
+ } finally {
+ evictCache();
+ }
}
@Override
public Result update(RevWalk walk) throws IOException {
- return evictCache(delegate.update(walk));
+ try {
+ return delegate.update(walk);
+ } finally {
+ evictCache();
+ }
}
@Override
public Result delete() throws IOException {
- return evictCache(delegate.delete());
+ try {
+ return delegate.delete();
+ } finally {
+ evictCache();
+ }
}
@Override
public Result delete(RevWalk walk) throws IOException {
- return evictCache(delegate.delete(walk));
+ try {
+ return delegate.delete(walk);
+ } finally {
+ evictCache();
+ }
}
@Override
public Result link(String target) throws IOException {
- return evictCache(delegate.link(target));
+ try {
+ return delegate.link(target);
+ } finally {
+ evictCache();
+ }
}
@Override
@@ -207,10 +228,7 @@
throw new UnsupportedOperationException(NOT_SUPPORTED_MSG);
}
- private Result evictCache(Result r) {
- if (SUCCESSFUL_UPDATES.contains(r)) {
- refsCache.evict(repo.getProjectName(), getName());
- }
- return r;
+ private void evictCache() {
+ refsCache.evict(repo.getProjectName(), getName());
}
}
