diff options
author | 2016-11-24 10:54:19 +0000 | |
---|---|---|
committer | 2016-11-24 13:33:26 +0000 | |
commit | 460db0f9487197754b8eaf66fb0c8da2bd7799f7 (patch) | |
tree | 7c94aed18531aec9b5578cfe0ee8cf8a25b77f79 /src/main/java/com/google/devtools | |
parent | 6450c187591bfbbd65766076f65f7c2901cc99bb (diff) |
Make SkylarkAttr.Descriptor thread-safe.
Also fixes thread-unsafety in AttributeBuilder.build.
--
MOS_MIGRATED_REVID=140118866
Diffstat (limited to 'src/main/java/com/google/devtools')
4 files changed, 58 insertions, 26 deletions
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java index 4f84481c99..a5ffdb9f63 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryModule.java @@ -21,7 +21,6 @@ import static com.google.devtools.build.lib.syntax.Type.STRING; import com.google.devtools.build.lib.analysis.BaseRuleClasses; import com.google.devtools.build.lib.cmdline.LabelSyntaxException; -import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.AttributeValueSource; import com.google.devtools.build.lib.packages.Package.NameConflictException; import com.google.devtools.build.lib.packages.PackageFactory; @@ -41,7 +40,6 @@ import com.google.devtools.build.lib.syntax.FunctionSignature; import com.google.devtools.build.lib.syntax.Runtime; import com.google.devtools.build.lib.syntax.SkylarkDict; import com.google.devtools.build.lib.syntax.SkylarkSignatureProcessor; - import java.util.Map; /** @@ -116,10 +114,9 @@ public class SkylarkRepositoryModule { for (Map.Entry<String, Descriptor> attr : castMap(attrs, String.class, Descriptor.class, "attrs").entrySet()) { Descriptor attrDescriptor = attr.getValue(); - AttributeValueSource source = attrDescriptor.getAttributeBuilder().getValueSource(); + AttributeValueSource source = attrDescriptor.getValueSource(); String attrName = source.convertToNativeName(attr.getKey(), ast.getLocation()); - Attribute.Builder<?> attrBuilder = attrDescriptor.getAttributeBuilder(); - builder.addOrOverrideAttribute(attrBuilder.build(attrName)); + builder.addOrOverrideAttribute(attrDescriptor.build(attrName)); } } builder.setConfiguredTargetFunction(implementation); diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java index db9d87f4b0..b1529fc4b0 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java +++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java @@ -1053,6 +1053,9 @@ public final class Attribute implements Comparable<Attribute> { Preconditions.checkState(!name.isEmpty(), "name has not been set"); // TODO(bazel-team): Set the default to be no file type, then remove this check, and also // remove all allowedFileTypes() calls without parameters. + + // do not modify this.allowedFileTypesForLabels, instead create a copy. + FileTypeSet allowedFileTypesForLabels = this.allowedFileTypesForLabels; if ((type == BuildType.LABEL) || (type == BuildType.LABEL_LIST)) { if ((name.startsWith("$") || name.startsWith(":")) && allowedFileTypesForLabels == null) { allowedFileTypesForLabels = FileTypeSet.ANY_FILE; diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java index 855e06f35a..d782ce3e4b 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java +++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkAttr.java @@ -25,6 +25,7 @@ import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.Attribute.AllowedValueSet; import com.google.devtools.build.lib.packages.Attribute.ConfigurationTransition; import com.google.devtools.build.lib.packages.Attribute.SkylarkComputedDefaultTemplate; +import com.google.devtools.build.lib.packages.AttributeValueSource; import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.SkylarkAspect; import com.google.devtools.build.lib.packages.SkylarkProviderIdentifier; @@ -1267,20 +1268,50 @@ public final class SkylarkAttr { public static final class Descriptor { private final Attribute.Builder<?> attributeBuilder; private final ImmutableList<SkylarkAspect> aspects; + + /** + * This lock guards {@code attributeBuilder} field. + * + * {@link Attribute.Builder} class is not thread-safe for concurrent modification. + * This class, together with its enclosing {@link SkylarkAttr} class, do not let + * anyone else access the {@code attributeBuilder}, however {@link #exportAspects(Location)} + * method actually modifies the {@code attributeBuilder}. Therefore all read- and write-accesses + * to {@code attributeBuilder} are protected with this lock. + * + * For example, {@link #hasDefault()} method only reads from {@link #attributeBuilder}, + * but we have no guarantee that it is safe to do so concurrently with adding aspects + * in {@link #exportAspects(Location)}. + */ + private final Object lock = new Object(); boolean exported; - public Descriptor(Attribute.Builder<?> attributeBuilder) { + private Descriptor(Attribute.Builder<?> attributeBuilder) { this(attributeBuilder, ImmutableList.<SkylarkAspect>of()); } - public Descriptor(Attribute.Builder<?> attributeBuilder, ImmutableList<SkylarkAspect> aspects) { + private Descriptor( + Attribute.Builder<?> attributeBuilder, ImmutableList<SkylarkAspect> aspects) { this.attributeBuilder = attributeBuilder; this.aspects = aspects; exported = false; } - public Attribute.Builder<?> getAttributeBuilder() { - return attributeBuilder; + public boolean hasDefault() { + synchronized (lock) { + return attributeBuilder.isValueSet(); + } + } + + public AttributeValueSource getValueSource() { + synchronized (lock) { + return attributeBuilder.getValueSource(); + } + } + + public Attribute build(String name) { + synchronized (lock) { + return attributeBuilder.build(name); + } } public ImmutableList<SkylarkAspect> getAspects() { @@ -1288,19 +1319,20 @@ public final class SkylarkAttr { } public void exportAspects(Location definitionLocation) throws EvalException { - if (exported) { - // Only export an attribute definiton once. - return; - } - Attribute.Builder<?> attributeBuilder = getAttributeBuilder(); - for (SkylarkAspect skylarkAspect : getAspects()) { - if (!skylarkAspect.isExported()) { - throw new EvalException(definitionLocation, - "All aspects applied to rule dependencies must be top-level values"); + synchronized (lock) { + if (exported) { + // Only export an attribute definiton once. + return; + } + for (SkylarkAspect skylarkAspect : getAspects()) { + if (!skylarkAspect.isExported()) { + throw new EvalException(definitionLocation, + "All aspects applied to rule dependencies must be top-level values"); + } + attributeBuilder.aspect(skylarkAspect, definitionLocation); } - attributeBuilder.aspect(skylarkAspect, definitionLocation); + exported = true; } - exported = true; } } diff --git a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java index c78aa186ab..9aa6b261a5 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java +++ b/src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleClassFunctions.java @@ -434,7 +434,7 @@ public class SkylarkRuleClassFunctions { for (Map.Entry<String, Descriptor> attr : castMap(attrs, String.class, Descriptor.class, "attrs").entrySet()) { Descriptor attrDescriptor = attr.getValue(); - AttributeValueSource source = attrDescriptor.getAttributeBuilder().getValueSource(); + AttributeValueSource source = attrDescriptor.getValueSource(); String attrName = source.convertToNativeName(attr.getKey(), ast.getLocation()); attributes.add(Pair.of(attrName, attrDescriptor)); } @@ -542,10 +542,10 @@ public class SkylarkRuleClassFunctions { attrObjectToAttributesList(attrs, ast); ImmutableList.Builder<Attribute> attributes = ImmutableList.builder(); ImmutableSet.Builder<String> requiredParams = ImmutableSet.<String>builder(); - for (Pair<String, Descriptor> descriptor : descriptors) { - String nativeName = descriptor.getFirst(); - boolean hasDefault = descriptor.getSecond().getAttributeBuilder().isValueSet(); - Attribute attribute = descriptor.second.getAttributeBuilder().build(descriptor.first); + for (Pair<String, Descriptor> nameDescriptorPair : descriptors) { + String nativeName = nameDescriptorPair.first; + boolean hasDefault = nameDescriptorPair.second.hasDefault(); + Attribute attribute = nameDescriptorPair.second.build(nameDescriptorPair.first); if (attribute.getType() == Type.STRING && ((String) attribute.getDefaultValue(null)).isEmpty()) { hasDefault = false; // isValueSet() is always true for attr.string. @@ -678,7 +678,7 @@ public class SkylarkRuleClassFunctions { descriptor.exportAspects(definitionLocation); addAttribute(definitionLocation, builder, - descriptor.getAttributeBuilder().build(attribute.getFirst())); + descriptor.build(attribute.getFirst())); } this.ruleClass = builder.build(ruleClassName); |