Make SkylarkAttr.Descriptor thread-safe.
Also fixes thread-unsafety in AttributeBuilder.build.
--
MOS_MIGRATED_REVID=140118866
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 4f84481..a5ffdb9 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 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.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 @@
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 db9d87f..b1529fc 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 @@
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 855e06f..d782ce3 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.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 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 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;
}
- attributeBuilder.aspect(skylarkAspect, definitionLocation);
+ 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);
+ }
+ 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 c78aa18..9aa6b26 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 @@
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 @@
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 @@
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 5009027..844a47d 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 @@
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 808ecd5..fb6550e 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 @@
.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 c5cf37a..b5cb1f6 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 @@
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 @@
@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 @@
@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 @@
@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 @@
@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 @@
@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 @@
}
@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 @@
@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 @@
@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 @@
@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();
