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(); } }