Make the builtins registry thread-safe

It was previously assumed that safety wasn't needed because

  1) all builtins should be registered in static initializer blocks, and
  2) all retrievals should occur during Skylark evaluation, after static initialization completes.

It turns out these assumptions aren't actually true (Who would've thunk it!). SkylarkActionFactory has been observed to be initialized as late as analysis time, and retrievals occur as early as constructing a PackageFactory (when scanning the native module). The failure mode is particularly ugly: Random Skylark method lookups will fail non-deterministically.

This change guards against this by making the builtins registry implement a form of freezing. Before freezing, reads and writes are allowed and are synchronized. After freezing, only reads are allowed and they are unsynchronized for performance. BlazeRuntime is responsible for flipping the bit, and for ensuring classes like SkylarkActionFactory run their initialization by that point. Unit tests don't need to worry, since they just stay unfrozen and synchronized throughout.

RELNOTES: None
PiperOrigin-RevId: 188080136
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Runtime.java b/src/main/java/com/google/devtools/build/lib/syntax/Runtime.java
index 3decbeb..2c7b0ee 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Runtime.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Runtime.java
@@ -160,11 +160,24 @@
    * A registry of builtins, including both global builtins and builtins that are under some
    * namespace.
    *
-   * <p>This object is unsynchronized, but concurrent reads are fine.
+   * <p>Concurrency model: This object is thread-safe. Read accesses are always allowed, while write
+   * accesses are only allowed before this object has been frozen ({@link #freeze}). Prior to
+   * freezing, all operations are synchronized, while after freezing they are lockless.
    */
   public static class BuiltinRegistry {
 
     /**
+     * Whether the registry's construction has completed.
+     *
+     * <p>Mutating methods may only be called while this is still false. Accessor methods may be
+     * called at any time.
+     *
+     * <p>We use {@code volatile} rather than {@link AtomicBoolean} because the bit flip only
+     * happens once, and doesn't require correlated reads and writes.
+     */
+    private volatile boolean frozen = false;
+
+    /**
      * All registered builtins, keyed and sorted by an identifying (but otherwise unimportant)
      * string.
      *
@@ -177,13 +190,36 @@
     /** All non-global builtin functions, keyed by their namespace class and their name. */
     private final Map<Class<?>, Map<String, BaseFunction>> functions = new HashMap<>();
 
+    /**
+     * Marks the registry as initialized, if it wasn't already.
+     *
+     * <p>It is guaranteed that after this method returns, all accessor methods are safe without
+     * synchronization; i.e. no mutation operation can touch the data structures.
+     */
+    public void freeze() {
+      // Similar to double-checked locking, but no need to check again on the inside since we don't
+      // care if two threads set the bit at once. The synchronized block is only to provide
+      // exclusion with mutations.
+      if (!this.frozen) {
+        synchronized (this) {
+          this.frozen = true;
+        }
+      }
+    }
+
     /** Registers a builtin with the given simple name, that was defined in the given Java class. */
-    public void registerBuiltin(Class<?> definingClass, String name, Object builtin) {
+    public synchronized void registerBuiltin(Class<?> definingClass, String name, Object builtin) {
       String key = String.format("%s.%s", definingClass.getName(), name);
       Preconditions.checkArgument(
           !allBuiltins.containsKey(key),
           "Builtin '%s' registered multiple times",
           key);
+
+      Preconditions.checkState(
+          !frozen,
+          "Attempted to register builtin '%s' after registry has already been frozen",
+          key);
+
       allBuiltins.put(key, builtin);
     }
 
@@ -192,7 +228,7 @@
      *
      * <p>This is independent of {@link #registerBuiltin}.
      */
-    public void registerFunction(Class<?> namespace, BaseFunction function) {
+    public synchronized void registerFunction(Class<?> namespace, BaseFunction function) {
       Preconditions.checkNotNull(namespace);
       Preconditions.checkNotNull(function.getObjectType());
       Class<?> skylarkNamespace = getSkylarkNamespace(namespace);
@@ -200,13 +236,26 @@
       Class<?> objType = getSkylarkNamespace(function.getObjectType());
       Preconditions.checkArgument(objType.equals(skylarkNamespace));
 
+      Preconditions.checkState(
+          !frozen,
+          "Attempted to register function '%s' in namespace '%s' after registry has already been "
+              + "frozen",
+          function,
+          namespace);
+
       functions.computeIfAbsent(namespace, k -> new HashMap<>());
       functions.get(namespace).put(function.getName(), function);
     }
 
     /** Returns a list of all registered builtins, in a deterministic order. */
     public ImmutableList<Object> getBuiltins() {
-      return ImmutableList.copyOf(allBuiltins.values());
+      if (frozen) {
+        return ImmutableList.copyOf(allBuiltins.values());
+      } else {
+        synchronized (this) {
+          return ImmutableList.copyOf(allBuiltins.values());
+        }
+      }
     }
 
     @Nullable
@@ -220,8 +269,15 @@
      * <p>If the namespace does not exist or has no function with that name, returns null.
      */
     public BaseFunction getFunction(Class<?> namespace, String name) {
-      Map<String, BaseFunction> namespaceFunctions = getFunctionsInNamespace(namespace);
-      return namespaceFunctions != null ? namespaceFunctions.get(name) : null;
+      if (frozen) {
+        Map<String, BaseFunction> namespaceFunctions = getFunctionsInNamespace(namespace);
+        return namespaceFunctions != null ? namespaceFunctions.get(name) : null;
+      } else {
+        synchronized (this) {
+          Map<String, BaseFunction> namespaceFunctions = getFunctionsInNamespace(namespace);
+          return namespaceFunctions != null ? namespaceFunctions.get(name) : null;
+        }
+      }
     }
 
     /**
@@ -230,11 +286,21 @@
      * <p>If the namespace does not exist, returns an empty set.
      */
     public ImmutableSet<String> getFunctionNames(Class<?> namespace) {
-      Map<String, BaseFunction> namespaceFunctions = getFunctionsInNamespace(namespace);
-      if (namespaceFunctions == null) {
-        return ImmutableSet.of();
+      if (frozen) {
+        Map<String, BaseFunction> namespaceFunctions = getFunctionsInNamespace(namespace);
+        if (namespaceFunctions == null) {
+          return ImmutableSet.of();
+        }
+        return ImmutableSet.copyOf(namespaceFunctions.keySet());
+      } else {
+        synchronized (this) {
+          Map<String, BaseFunction> namespaceFunctions = getFunctionsInNamespace(namespace);
+          if (namespaceFunctions == null) {
+            return ImmutableSet.of();
+          }
+          return ImmutableSet.copyOf(namespaceFunctions.keySet());
+        }
       }
-      return ImmutableSet.copyOf(namespaceFunctions.keySet());
     }
   }