Make SkylarkAttr.Descriptor thread-safe.

Also fixes thread-unsafety in AttributeBuilder.build.

--
MOS_MIGRATED_REVID=140118866

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