Fix race condition in `RuleConfiguredTargetValue.clear`.
Also create a common base class for RuleConfiguredTargetValue and NonRuleConfiguredTargetValue, so that code doesn't need to be duplicated.
PiperOrigin-RevId: 624227864
Change-Id: I92dd138f662422d765e5ba927a1cf9312bb90078
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
index 325ec13..7ed740b 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -3124,6 +3124,7 @@
java_library(
name = "rule_configured_target_value",
srcs = [
+ "BaseRuleConfiguredTargetValue.java",
"NonRuleConfiguredTargetValue.java",
"RuleConfiguredTargetValue.java",
],
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BaseRuleConfiguredTargetValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/BaseRuleConfiguredTargetValue.java
new file mode 100644
index 0000000..b7a11df
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BaseRuleConfiguredTargetValue.java
@@ -0,0 +1,72 @@
+// Copyright 2024 The Bazel Authors. All rights reserved.
+//
+// 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.devtools.build.lib.skyframe;
+
+import static com.google.devtools.build.lib.analysis.config.CommonOptions.EMPTY_OPTIONS;
+
+import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.analysis.ConfiguredTarget;
+import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet;
+import com.google.devtools.build.lib.packages.Package;
+import javax.annotation.Nullable;
+
+/** Common base class for configured target values for rules and non-rules. */
+abstract class BaseRuleConfiguredTargetValue<T extends ConfiguredTarget>
+ implements ConfiguredTargetValue {
+ // This variable is non-final because it may be clear()ed to save memory. It is null only after
+ // clear(true) is called.
+ @Nullable private T configuredTarget;
+
+ // May be null after clearing; because transitive packages are not tracked; or after
+ // deserialization.
+ @Nullable private transient NestedSet<Package> transitivePackages;
+
+ BaseRuleConfiguredTargetValue(
+ T configuredTarget, @Nullable NestedSet<Package> transitivePackages) {
+ this.configuredTarget = Preconditions.checkNotNull(configuredTarget);
+ this.transitivePackages = transitivePackages;
+ }
+
+ @Nullable // May be null after clearing.
+ @Override
+ public T getConfiguredTarget() {
+ return configuredTarget;
+ }
+
+ @Nullable
+ @Override
+ public NestedSet<Package> getTransitivePackages() {
+ return transitivePackages;
+ }
+
+ @Override
+ public void clear(boolean clearEverything) {
+ // Snapshot this because this method is sometimes called from multiple threads. We don't need
+ // to actually synchronize: both threads will have the same logic and so there's no risk of
+ // a race condition.
+ T ct = this.configuredTarget;
+ if (ct != null
+ && ct.getConfigurationKey() != null
+ && ct.getConfigurationChecksum().equals(EMPTY_OPTIONS.checksum())) {
+ // Keep these to avoid the need to re-create them later, they are dependencies of the empty
+ // configuration key and will never change.
+ return;
+ }
+ if (clearEverything) {
+ this.configuredTarget = null;
+ }
+ this.transitivePackages = null;
+ }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/NonRuleConfiguredTargetValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/NonRuleConfiguredTargetValue.java
index cfaaaee..856256e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/NonRuleConfiguredTargetValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/NonRuleConfiguredTargetValue.java
@@ -13,10 +13,8 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
-import static com.google.devtools.build.lib.analysis.config.CommonOptions.EMPTY_OPTIONS;
import com.google.common.base.MoreObjects;
-import com.google.common.base.Preconditions;
import com.google.devtools.build.lib.analysis.ConfiguredTarget;
import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
@@ -33,13 +31,8 @@
@ThreadSafe
// Reached via OutputFileConfiguredTarget.
@AutoCodec(explicitlyAllowClass = RuleConfiguredTarget.class)
-public final class NonRuleConfiguredTargetValue implements ConfiguredTargetValue {
- // These variables are only non-final because they may be clear()ed to save memory.
- // configuredTarget is null only after it is cleared.
- @Nullable private ConfiguredTarget configuredTarget;
-
- // May be null either after clearing or because transitive packages are not tracked.
- @Nullable private NestedSet<Package> transitivePackages;
+public final class NonRuleConfiguredTargetValue
+ extends BaseRuleConfiguredTargetValue<ConfiguredTarget> implements ConfiguredTargetValue {
@AutoCodec.Instantiator
@VisibleForSerialization
@@ -51,39 +44,13 @@
NonRuleConfiguredTargetValue(
ConfiguredTarget configuredTarget, @Nullable NestedSet<Package> transitivePackages) {
- this.configuredTarget = Preconditions.checkNotNull(configuredTarget);
- this.transitivePackages = transitivePackages;
- }
-
- @Nullable // May be null after clearing.
- @Override
- public ConfiguredTarget getConfiguredTarget() {
- return configuredTarget;
- }
-
- @Nullable
- @Override
- public NestedSet<Package> getTransitivePackages() {
- return transitivePackages;
- }
-
- @Override
- public void clear(boolean clearEverything) {
- if (configuredTarget != null
- && configuredTarget.getConfigurationKey() != null
- && configuredTarget.getConfigurationChecksum().equals(EMPTY_OPTIONS.checksum())) {
- // Keep these to avoid the need to re-create them later, they are dependencies of the empty
- // configuration key and will never change.
- return;
- }
- if (clearEverything) {
- configuredTarget = null;
- }
- transitivePackages = null;
+ super(configuredTarget, transitivePackages);
}
@Override
public String toString() {
- return MoreObjects.toStringHelper(this).add("configuredTarget", configuredTarget).toString();
+ return MoreObjects.toStringHelper(this)
+ .add("configuredTarget", getConfiguredTarget())
+ .toString();
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/RuleConfiguredTargetValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/RuleConfiguredTargetValue.java
index 3a8ae0d..14f05b6 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/RuleConfiguredTargetValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/RuleConfiguredTargetValue.java
@@ -14,10 +14,8 @@
package com.google.devtools.build.lib.skyframe;
import static com.google.common.base.Preconditions.checkNotNull;
-import static com.google.devtools.build.lib.analysis.config.CommonOptions.EMPTY_OPTIONS;
import com.google.common.base.MoreObjects;
-import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.analysis.ConfiguredTargetValue;
@@ -33,12 +31,9 @@
@Immutable
@ThreadSafe
public final class RuleConfiguredTargetValue
+ extends BaseRuleConfiguredTargetValue<RuleConfiguredTarget>
implements RuleConfiguredObjectValue, ConfiguredTargetValue {
- // This variable is non-final because it may be clear()ed to save memory. It is null only after
- // clear(true) is called.
- @Nullable private RuleConfiguredTarget configuredTarget;
-
/**
* Operations accessing actions, for example, executing them, should be performed in the same
* Bazel instance that constructs the {@code RuleConfiguredTargetValue} instance and not on a
@@ -47,55 +42,23 @@
@Nullable // Null if deserialized.
private final transient ImmutableList<ActionAnalysisMetadata> actions;
- // May be null after clearing; because transitive packages are not tracked; or after
- // deserialization.
- @Nullable private transient NestedSet<Package> transitivePackages;
-
public RuleConfiguredTargetValue(
RuleConfiguredTarget configuredTarget, @Nullable NestedSet<Package> transitivePackages) {
- this.configuredTarget = Preconditions.checkNotNull(configuredTarget);
- this.transitivePackages = transitivePackages;
+ super(configuredTarget, transitivePackages);
// These are specifically *not* copied to save memory.
this.actions = configuredTarget.getActions();
}
- @Nullable // May be null after clearing.
- @Override
- public RuleConfiguredTarget getConfiguredTarget() {
- return configuredTarget;
- }
-
@Override
public ImmutableList<ActionAnalysisMetadata> getActions() {
return checkNotNull(actions, "actions are not available on deserialized instances");
}
- @Nullable
- @Override
- public NestedSet<Package> getTransitivePackages() {
- return transitivePackages;
- }
-
- @Override
- public void clear(boolean clearEverything) {
- if (configuredTarget != null
- && configuredTarget.getConfigurationKey() != null
- && configuredTarget.getConfigurationChecksum().equals(EMPTY_OPTIONS.checksum())) {
- // Keep these to avoid the need to re-create them later, they are dependencies of the empty
- // configuration key and will never change.
- return;
- }
- if (clearEverything) {
- configuredTarget = null;
- }
- transitivePackages = null;
- }
-
@Override
public String toString() {
return MoreObjects.toStringHelper(this)
.add("actions", actions)
- .add("configuredTarget", configuredTarget)
+ .add("configuredTarget", getConfiguredTarget())
.toString();
}
}
