diff options
author | Dmitry Lomov <dslomov@google.com> | 2016-11-24 10:54:19 +0000 |
---|---|---|
committer | Dmitry Lomov <dslomov@google.com> | 2016-11-24 13:33:26 +0000 |
commit | 460db0f9487197754b8eaf66fb0c8da2bd7799f7 (patch) | |
tree | 7c94aed18531aec9b5578cfe0ee8cf8a25b77f79 | |
parent | 6450c187591bfbbd65766076f65f7c2901cc99bb (diff) |
Make SkylarkAttr.Descriptor thread-safe.
Also fixes thread-unsafety in AttributeBuilder.build.
--
MOS_MIGRATED_REVID=140118866
7 files changed, 121 insertions, 65 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); diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java index 5009027f14..844a47df89 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkAspectsTest.java @@ -1615,7 +1615,42 @@ public class SkylarkAspectsTest extends AnalysisTestCase { assertThat(result).containsExactly("//test:r0=no", "//test:r1=no", "//test:r2_1=yes"); } + @Test + public void attributesWithAspectsReused() throws Exception { + scratch.file( + "test/aspect.bzl", + "def _impl(target, ctx):", + " return struct()", + "my_aspect = aspect(_impl)", + "a_dict = { 'foo' : attr.label_list(aspects = [my_aspect]) }" + ); + + scratch.file( + "test/r1.bzl", + "load(':aspect.bzl', 'my_aspect', 'a_dict')", + "def _rule_impl(ctx):", + " pass", + "r1 = rule(_rule_impl, attrs = a_dict)" + ); + scratch.file( + "test/r2.bzl", + "load(':aspect.bzl', 'my_aspect', 'a_dict')", + "def _rule_impl(ctx):", + " pass", + "r2 = rule(_rule_impl, attrs = a_dict)" + ); + + scratch.file( + "test/BUILD", + "load(':r1.bzl', 'r1')", + "load(':r2.bzl', 'r2')", + "r1(name = 'x1')", + "r2(name = 'x2', foo = [':x1'])" + ); + AnalysisResult analysisResult = update("//test:x2"); + assertThat(analysisResult.hasError()).isFalse(); + } @RunWith(JUnit4.class) public static final class WithKeepGoing extends SkylarkAspectsTest { diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java index 808ecd59f0..fb6550eafb 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkIntegrationTest.java @@ -259,8 +259,6 @@ public class SkylarkIntegrationTest extends BuildViewTestCase { .containsExactlyElementsIn(hiddenTopLevelArtifacts); } - - @Test public void testOutputGroupsWithList() throws Exception { scratch.file( diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java index c5cf37a0c1..b5cb1f652e 100644 --- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java +++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleClassFunctionsTest.java @@ -138,50 +138,49 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase { eval("def impl():\n" + " return 0\n"); } - private Attribute.Builder<?> evalAttributeDefinition(String... lines) throws Exception { - return ((SkylarkAttr.Descriptor) evalRuleClassCode(lines)).getAttributeBuilder(); - } - @Test public void testAttrWithOnlyType() throws Exception { - Attribute attr = evalAttributeDefinition("attr.string_list()").build("a1"); + Attribute attr = buildAttribute("a1", "attr.string_list()", ""); assertEquals(Type.STRING_LIST, attr.getType()); } + private Attribute buildAttribute(String name, String... lines) throws Exception { + return ((SkylarkAttr.Descriptor) evalRuleClassCode(lines)).build(name); + } + @Test public void testOutputListAttr() throws Exception { - Attribute attr = evalAttributeDefinition("attr.output_list()").build("a1"); + Attribute attr = buildAttribute("a1", "attr.output_list()"); assertEquals(BuildType.OUTPUT_LIST, attr.getType()); } @Test public void testIntListAttr() throws Exception { - Attribute attr = evalAttributeDefinition("attr.int_list()").build("a1"); + Attribute attr = buildAttribute("a1", "attr.int_list()"); assertEquals(Type.INTEGER_LIST, attr.getType()); } @Test public void testOutputAttr() throws Exception { - Attribute attr = evalAttributeDefinition("attr.output()").build("a1"); + Attribute attr = buildAttribute("a1", "attr.output()"); assertEquals(BuildType.OUTPUT, attr.getType()); } @Test public void testStringDictAttr() throws Exception { - Attribute attr = evalAttributeDefinition("attr.string_dict(default = {'a': 'b'})").build("a1"); + Attribute attr = buildAttribute("a1", "attr.string_dict(default = {'a': 'b'})"); assertEquals(Type.STRING_DICT, attr.getType()); } @Test public void testStringListDictAttr() throws Exception { - Attribute attr = evalAttributeDefinition("attr.string_list_dict(default = {'a': ['b', 'c']})") - .build("a1"); + Attribute attr = buildAttribute("a1", "attr.string_list_dict(default = {'a': ['b', 'c']})"); assertEquals(Type.STRING_LIST_DICT, attr.getType()); } @Test public void testAttrAllowedFileTypesAnyFile() throws Exception { - Attribute attr = evalAttributeDefinition("attr.label_list(allow_files = True)").build("a1"); + Attribute attr = buildAttribute("a1", "attr.label_list(allow_files = True)"); assertEquals(FileTypeSet.ANY_FILE, attr.getAllowedFileTypesPredicate()); } @@ -201,8 +200,7 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase { @Test public void testAttrWithList() throws Exception { - Attribute attr = evalAttributeDefinition("attr.label_list(allow_files = ['.xml'])") - .build("a1"); + Attribute attr = buildAttribute("a1", "attr.label_list(allow_files = ['.xml'])"); assertTrue(attr.getAllowedFileTypesPredicate().apply("a.xml")); assertFalse(attr.getAllowedFileTypesPredicate().apply("a.txt")); assertFalse(attr.isSingleArtifact()); @@ -210,8 +208,7 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase { @Test public void testAttrSingleFileWithList() throws Exception { - Attribute attr = evalAttributeDefinition("attr.label(allow_single_file = ['.xml'])") - .build("a1"); + Attribute attr = buildAttribute("a1", "attr.label(allow_single_file = ['.xml'])"); assertTrue(attr.getAllowedFileTypesPredicate().apply("a.xml")); assertFalse(attr.getAllowedFileTypesPredicate().apply("a.txt")); assertTrue(attr.isSingleArtifact()); @@ -219,8 +216,7 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase { @Test public void testAttrWithSkylarkFileType() throws Exception { - Attribute attr = evalAttributeDefinition("attr.label_list(allow_files = FileType(['.xml']))") - .build("a1"); + Attribute attr = buildAttribute("a1", "attr.label_list(allow_files = FileType(['.xml']))"); assertTrue(attr.getAllowedFileTypesPredicate().apply("a.xml")); assertFalse(attr.getAllowedFileTypesPredicate().apply("a.txt")); } @@ -228,8 +224,7 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase { @Test public void testAttrWithProviders() throws Exception { Attribute attr = - evalAttributeDefinition("attr.label_list(allow_files = True, providers = ['a', 'b'])") - .build("a1"); + buildAttribute("a1", "attr.label_list(allow_files = True, providers = ['a', 'b'])"); assertThat(attr.getMandatoryProvidersList()) .containsExactly(ImmutableSet.of(legacy("a"), legacy("b"))); } @@ -241,9 +236,8 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase { @Test public void testAttrWithProvidersList() throws Exception { Attribute attr = - evalAttributeDefinition("attr.label_list(allow_files = True," - + " providers = [['a', 'b'], ['c']])") - .build("a1"); + buildAttribute("a1", "attr.label_list(allow_files = True," + + " providers = [['a', 'b'], ['c']])"); assertThat(attr.getMandatoryProvidersList()).containsExactly( ImmutableSet.of(legacy("a"), legacy("b")), ImmutableSet.of(legacy("c"))); @@ -384,14 +378,14 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase { } @Test public void testAttrAllowedRuleClassesSpecificRuleClasses() throws Exception { - Attribute attr = evalAttributeDefinition( - "attr.label_list(allow_rules = ['java_binary'], allow_files = True)").build("a"); + Attribute attr = buildAttribute("a", + "attr.label_list(allow_rules = ['java_binary'], allow_files = True)"); assertTrue(attr.getAllowedRuleClassesPredicate().apply(ruleClass("java_binary"))); assertFalse(attr.getAllowedRuleClassesPredicate().apply(ruleClass("genrule"))); } @Test public void testAttrDefaultValue() throws Exception { - Attribute attr = evalAttributeDefinition("attr.string(default = 'some value')").build("a1"); + Attribute attr = buildAttribute("a1", "attr.string(default = 'some value')"); assertEquals("some value", attr.getDefaultValueForTesting()); } @@ -406,21 +400,21 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase { @Test public void testAttrMandatory() throws Exception { - Attribute attr = evalAttributeDefinition("attr.string(mandatory=True)").build("a1"); + Attribute attr = buildAttribute("a1", "attr.string(mandatory=True)"); assertTrue(attr.isMandatory()); assertFalse(attr.isNonEmpty()); } @Test public void testAttrNonEmpty() throws Exception { - Attribute attr = evalAttributeDefinition("attr.string_list(non_empty=True)").build("a1"); + Attribute attr = buildAttribute("a1", "attr.string_list(non_empty=True)"); assertTrue(attr.isNonEmpty()); assertFalse(attr.isMandatory()); } @Test public void testAttrAllowEmpty() throws Exception { - Attribute attr = evalAttributeDefinition("attr.string_list(allow_empty=False)").build("a1"); + Attribute attr = buildAttribute("a1", "attr.string_list(allow_empty=False)"); assertTrue(attr.isNonEmpty()); assertFalse(attr.isMandatory()); } @@ -433,28 +427,25 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase { @Test public void testAttrCfg() throws Exception { - Attribute attr = evalAttributeDefinition("attr.label(cfg = 'host', allow_files = True)") - .build("a1"); + Attribute attr = buildAttribute("a1", "attr.label(cfg = 'host', allow_files = True)"); assertEquals(ConfigurationTransition.HOST, attr.getConfigurationTransition()); } @Test public void testAttrCfgData() throws Exception { - Attribute attr = evalAttributeDefinition("attr.label(cfg = 'data', allow_files = True)") - .build("a1"); + Attribute attr = buildAttribute("a1", "attr.label(cfg = 'data', allow_files = True)"); assertEquals(ConfigurationTransition.DATA, attr.getConfigurationTransition()); } @Test public void testAttrCfgTarget() throws Exception { - Attribute attr = evalAttributeDefinition("attr.label(cfg = 'target', allow_files = True)") - .build("a1"); + Attribute attr = buildAttribute("a1", "attr.label(cfg = 'target', allow_files = True)"); assertEquals(ConfigurationTransition.NONE, attr.getConfigurationTransition()); } @Test public void testAttrValues() throws Exception { - Attribute attr = evalAttributeDefinition("attr.string(values = ['ab', 'cd'])").build("a1"); + Attribute attr = buildAttribute("a1", "attr.string(values = ['ab', 'cd'])"); PredicateWithMessage<Object> predicate = attr.getAllowedValues(); assertThat(predicate.apply("ab")).isTrue(); assertThat(predicate.apply("xy")).isFalse(); @@ -462,7 +453,7 @@ public class SkylarkRuleClassFunctionsTest extends SkylarkTestCase { @Test public void testAttrIntValues() throws Exception { - Attribute attr = evalAttributeDefinition("attr.int(values = [1, 2])").build("a1"); + Attribute attr = buildAttribute("a1", "attr.int(values = [1, 2])"); PredicateWithMessage<Object> predicate = attr.getAllowedValues(); assertThat(predicate.apply(2)).isTrue(); assertThat(predicate.apply(3)).isFalse(); |