Clean up interning pools in various places.

Send a bug report if interning pools are not cleaned up outside of Java tests.

PiperOrigin-RevId: 527093155
Change-Id: I7b53ddd3a5b3481e504cc9ccc97231298e68fd60

src/main/java/com/google/devtools/build/lib/skyframe/packages/AbstractPackageLoader.java

@@ -369,8 +369,15 @@ private Result loadPackagesInternal(
       EvaluationContext evaluationContext,
       StoredEventHandler storedEventHandler)
       throws InterruptedException {
-    EvaluationResult<PackageValue> evalResult =
-        makeFreshEvaluator().evaluate(pkgKeys, evaluationContext);
+    MemoizingEvaluator evaluator = makeFreshEvaluator();
+    EvaluationResult<PackageValue> evalResult;
+    try {
+      evalResult = evaluator.evaluate(pkgKeys, evaluationContext);
+    } finally {
+      if (usePooledInterning) {
+        evaluator.cleanupInterningPools();
+      }
+    }
     ImmutableMap.Builder<PackageIdentifier, PackageLoader.PackageOrException> result =
         ImmutableMap.builder();
     for (SkyKey key : pkgKeys) {

src/main/java/com/google/devtools/build/skyframe/BUILD

@@ -26,10 +26,12 @@ java_library(
     srcs = SKYFRAME_OBJECT_SRCS,
     visibility = ["//visibility:public"],
     deps = [
+        "//src/main/java/com/google/devtools/build/lib/bugreport",
         "//src/main/java/com/google/devtools/build/lib/collect/compacthashset",
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec",
         "//src/main/java/com/google/devtools/build/lib/skyframe/serialization/autocodec:serialization-constant",
+        "//src/main/java/com/google/devtools/build/lib/util:TestType",
         "//third_party:auto_value",
         "//third_party:checker_framework_annotations",
         "//third_party:guava",

src/main/java/com/google/devtools/build/skyframe/SkyKey.java

@@ -13,8 +13,10 @@
 // limitations under the License.
 package com.google.devtools.build.skyframe;
 
+import com.google.devtools.build.lib.bugreport.BugReport;
 import com.google.devtools.build.lib.concurrent.PooledInterner;
 import com.google.devtools.build.lib.concurrent.ThreadSafety;
+import com.google.devtools.build.lib.util.TestType;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import java.io.Serializable;
 import javax.annotation.Nullable;
@@ -81,13 +83,19 @@ final class SkyKeyInterner<T extends SkyKey> extends PooledInterner<T> {
     /**
      * Sets the {@link Pool} to be used for interning.
      *
-     * <p>The pool is strongly retained until another pool is set. {@code null} can be passed to
-     * clear the global pool.
+     * <p>The pool is strongly retained until it is cleared, which can be accomplished by passing
+     * {@code null} to this method.
      */
     @ThreadSafety.ThreadCompatible
-    public static void setGlobalPool(@Nullable Pool<SkyKey> pool) {
+    static void setGlobalPool(@Nullable Pool<SkyKey> pool) {
       // No synchronization is needed. Setting global pool is guaranteed to happen sequentially
       // since only one build can happen at the same time.
+      if (pool != null
+          && globalPool != null
+          && (!TestType.isInTest() || TestType.getTestType() == TestType.SHELL_INTEGRATION)) {
+        BugReport.sendNonFatalBugReport(
+            new IllegalStateException("Global SkyKey pool not cleared before setting another"));
+      }
       globalPool = pool;
     }
 
@@ -104,7 +112,7 @@ protected Pool<T> getPool() {
      */
     @CanIgnoreReturnValue
     @SuppressWarnings("unchecked")
-    public T weakInternUnchecked(SkyKey sample) {
+    T weakInternUnchecked(SkyKey sample) {
       return weakIntern((T) sample);
     }
   }

src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java

@@ -104,6 +104,7 @@
 import java.util.Optional;
 import java.util.Set;
 import java.util.UUID;
+import org.junit.After;
 import org.junit.Before;
 
 /**
@@ -211,7 +212,12 @@ public final void createMocks() throws Exception {
     useRuleClassProvider(analysisMock.createRuleClassProvider());
   }
 
-  protected SkyframeExecutor createSkyframeExecutor(PackageFactory pkgFactory) {
+  @After
+  public final void cleanupInterningPools() {
+    skyframeExecutor.getEvaluator().cleanupInterningPools();
+  }
+
+  private SkyframeExecutor createSkyframeExecutor(PackageFactory pkgFactory) {
     return BazelSkyframeExecutorConstants.newBazelSkyframeExecutorBuilder()
         .setPkgFactory(pkgFactory)
         .setFileSystem(fileSystem)

src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java

@@ -206,6 +206,7 @@
 import java.util.regex.Pattern;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.StarlarkSemantics;
+import org.junit.After;
 import org.junit.Before;
 
 /** Common test code that creates a BuildView instance. */
@@ -243,6 +244,11 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
   private ActionLogBufferPathGenerator actionLogBufferPathGenerator;
 
+  @After
+  public final void cleanupInterningPools() {
+    skyframeExecutor.getEvaluator().cleanupInterningPools();
+  }
+
   @Before
   public final void initializeSkyframeExecutor() throws Exception {
     initializeSkyframeExecutor(/*doPackageLoadingChecks=*/ true);
@@ -2537,7 +2543,7 @@ public ActionExecutionContext build() {
           /* topLevelFilesets= */ ImmutableMap.of(),
           artifactExpander,
           /* actionFileSystem= */ null,
-          /*skyframeDepsResult*/ null,
+          /* skyframeDepsResult= */ null,
           DiscoveredModulesPruner.DEFAULT,
           SyscallCache.NO_CACHE,
           ThreadStateReceiver.NULL_INSTANCE);

src/test/java/com/google/devtools/build/lib/buildtool/util/BuildIntegrationTestCase.java

@@ -303,6 +303,9 @@ protected ServerDirectories createServerDirectories() {
   }
 
   protected void createRuntimeWrapper() throws Exception {
+    if (runtimeWrapper != null) {
+      cleanupInterningPools();
+    }
     runtimeWrapper =
         new BlazeRuntimeWrapper(
             events,
@@ -351,6 +354,11 @@ protected void runPriorToBeforeMethods() throws Exception {
     // methods are being run.
   }
 
+  @After
+  public final void cleanupInterningPools() {
+    getSkyframeExecutor().getEvaluator().cleanupInterningPools();
+  }
+
   @After
   public final void cleanUp() throws Exception {
     if (subscriberException.getException() != null) {

src/test/java/com/google/devtools/build/skyframe/ParallelEvaluatorTest.java

@@ -28,6 +28,7 @@
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyNoMoreInteractions;
 
@@ -386,7 +387,7 @@ public void interruptedEvaluatorThreadAfterEnqueueBeforeWaitForCompletionAndCons
     // thread, aka the main Skyframe evaluation thread),
     CountDownLatch keyAStartedComputingLatch = new CountDownLatch(1);
     CountDownLatch keyBAddReverseDepAndCheckIfDoneLatch = new CountDownLatch(1);
-    InMemoryNodeEntry nodeEntryB = mock(InMemoryNodeEntry.class);
+    InMemoryNodeEntry nodeEntryB = spy(new InMemoryNodeEntry(keyB));
     AtomicBoolean keyBAddReverseDepAndCheckIfDoneInterrupted = new AtomicBoolean(false);
     doAnswer(
             invocation -> {
@@ -2735,7 +2736,7 @@ public void raceConditionWithNoKeepGoingErrors_FutureError() throws Exception {
                 // 'otherParentKey'. This test case is testing for a real race condition and the
                 // 10ms time was chosen experimentally to give a true positive rate of 99.8%
                 // (without a sleep it has a 1% true positive rate). There's no good way to do
-                // this without sleeping. We *could* introspect ParallelEvaulator's
+                // this without sleeping. We *could* introspect ParallelEvaluator's
                 // AbstractQueueVisitor to see if the re-evaluation has been enqueued, but that's
                 // relying on pretty low-level implementation details.
                 Uninterruptibles.sleepUninterruptibly(10, TimeUnit.MILLISECONDS);