ChangedFiles: Check experiment flag only when current user is available
Checking whether the experiment flag for using the diff cache is set
requires a current user, at least with the Google implementation of
experiment flags. If a current user is not available a warning is logged
and the experiment flag is considered as not set. Since ChangedFiles is
called from a background thread regularly (e.g. when indexing a change)
we see this warning frequently. Fix this by checking for the experiment
flag only when a current user is available, otherwise return false to
not use the diff cache in this case.
Alternatively it was considered to make a current user available by
opening a ManualRequestContext, but we don't have a suitable user
available that should be provided as current user and providing the
internal user would always evaluate to false, since experiment flags are
never set for the internal user. This means we better directly return
false in this case.
Bug: Google b/191961988
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ia0359f5e8ea661df734b53739772e4b2282e2bcc
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
index b3c8404..127377f 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
@@ -40,7 +40,9 @@
import com.google.gerrit.server.patch.DiffNotAvailableException;
import com.google.gerrit.server.patch.DiffOperations;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import com.google.gerrit.server.util.ThreadLocalRequestContext;
import com.google.inject.Inject;
+import com.google.inject.OutOfScopeException;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.io.IOException;
@@ -88,6 +90,7 @@
private final Provider<AutoMerger> autoMergerProvider;
private final CodeOwnerMetrics codeOwnerMetrics;
private final ThreeWayMergeStrategy mergeStrategy;
+ private final ThreadLocalRequestContext threadLocalRequestContext;
private final ExperimentFeatures experimentFeatures;
@Inject
@@ -98,12 +101,14 @@
DiffOperations diffOperations,
Provider<AutoMerger> autoMergerProvider,
CodeOwnerMetrics codeOwnerMetrics,
+ ThreadLocalRequestContext threadLocalRequestContext,
ExperimentFeatures experimentFeatures) {
this.repoManager = repoManager;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.diffOperations = diffOperations;
this.autoMergerProvider = autoMergerProvider;
this.codeOwnerMetrics = codeOwnerMetrics;
+ this.threadLocalRequestContext = threadLocalRequestContext;
this.experimentFeatures = experimentFeatures;
this.mergeStrategy = MergeUtil.getMergeStrategy(cfg);
}
@@ -129,12 +134,27 @@
requireNonNull(revision, "revision");
requireNonNull(mergeCommitStrategy, "mergeCommitStrategy");
- if (experimentFeatures.isFeatureEnabled(CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE)) {
+ if (useDiffCache()) {
return getFromDiffCache(project, revision, mergeCommitStrategy);
}
return compute(project, revision, mergeCommitStrategy);
}
+ /** Checks whether the experiment flag to use the diff cache is set. */
+ private boolean useDiffCache() {
+ // Checking whether the experiment flag is set requires a current user, but since this code
+ // may be called from a background thread (e.g. when indexing a change) a current user may not
+ // be available. Hence check for the experiment flag only when a current user is available,
+ // otherwise return false to not use the diff cache in this case.
+ try {
+ threadLocalRequestContext.getContext().getUser();
+ } catch (OutOfScopeException e) {
+ return false;
+ }
+ return experimentFeatures.isFeatureEnabled(
+ CodeOwnersExperimentFeaturesConstants.USE_DIFF_CACHE);
+ }
+
/**
* Returns the changed files for the given revision.
*
