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

1 files changed, 25 insertions, 0 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/syntax/RuntimeTest.java b/src/test/java/com/google/devtools/build/lib/syntax/RuntimeTest.java
index 60df36ba70..9da686707a 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/RuntimeTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/RuntimeTest.java
@@ -15,6 +15,7 @@
 package com.google.devtools.build.lib.syntax;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.devtools.build.lib.testutil.MoreAsserts.assertThrows;
 
 import com.google.devtools.build.lib.skylarkinterface.SkylarkPrinter;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
@@ -83,6 +84,30 @@ public final class RuntimeTest {
   }
 
   @Test
+  public void checkRegistry_WriteAfterFreezeFails_Builtin() {
+    Runtime.BuiltinRegistry reg = new Runtime.BuiltinRegistry();
+    reg.freeze();
+    IllegalStateException expected = assertThrows(
+        IllegalStateException.class,
+        () -> reg.registerBuiltin(DummyType.class, "dummy", "foo"));
+    assertThat(expected).hasMessageThat()
+        .matches("Attempted to register builtin '(.*)DummyType.dummy' after registry has already "
+            + "been frozen");
+  }
+
+  @Test
+  public void checkRegistry_WriteAfterFreezeFails_Function() {
+    Runtime.BuiltinRegistry reg = new Runtime.BuiltinRegistry();
+    reg.freeze();
+    IllegalStateException expected = assertThrows(
+        IllegalStateException.class,
+        () -> reg.registerFunction(DummyType.class, DUMMY_FUNC));
+    assertThat(expected).hasMessageThat()
+        .matches("Attempted to register function 'dummyFunc' in namespace '(.*)DummyType' after "
+            + "registry has already been frozen");
+  }
+
+  @Test
   public void checkStaticallyRegistered_Method() throws Exception {
     Field splitField = MethodLibrary.class.getDeclaredField("split");
     splitField.setAccessible(true);