Add more debugging for bug: put current invocation id in LabelVisitor threads, so that if a lingering thread from a prior invocation is responsible for a re-entrant evaluation, we'll see.

PiperOrigin-RevId: 423418130

Author: janakdr

src/main/java/com/google/devtools/build/lib/query2/QueryEnvironmentFactory.java

@@ -56,7 +56,8 @@ public AbstractBlazeQueryEnvironment<Target> create(
       Iterable<QueryFunction> extraFunctions,
       @Nullable PathPackageLocator packagePath,
       boolean blockUniverseEvaluationErrors,
-      boolean useGraphlessQuery) {
+      boolean useGraphlessQuery,
+      String queryIdForDebugging) {
     Preconditions.checkNotNull(universeScope);
     if (canUseSkyQuery(orderedResults, universeScope, packagePath, strictScope, labelFilter)) {
       return new SkyQueryEnvironment(
@@ -83,7 +84,8 @@ public AbstractBlazeQueryEnvironment<Target> create(
           labelFilter,
           eventHandler,
           settings,
-          extraFunctions);
+          extraFunctions,
+          queryIdForDebugging);
     } else {
       return new BlazeQueryEnvironment(
           queryTransitivePackagePreloader,
@@ -97,7 +99,8 @@ public AbstractBlazeQueryEnvironment<Target> create(
           labelFilter,
           eventHandler,
           settings,
-          extraFunctions);
+          extraFunctions,
+          queryIdForDebugging);
     }
   }
 

src/main/java/com/google/devtools/build/lib/query2/query/BlazeQueryEnvironment.java

@@ -113,7 +113,8 @@ public BlazeQueryEnvironment(
       Predicate<Label> labelFilter,
       ExtendedEventHandler eventHandler,
       Set<Setting> settings,
-      Iterable<QueryFunction> extraFunctions) {
+      Iterable<QueryFunction> extraFunctions,
+      String queryIdForDebugging) {
     super(keepGoing, strictScope, labelFilter, eventHandler, settings, extraFunctions);
     this.targetPatternPreloader = targetPatternPreloader;
     this.relativeWorkingDirectory = relativeWorkingDirectory;
@@ -122,7 +123,7 @@ public BlazeQueryEnvironment(
     this.cachingPackageLocator = cachingPackageLocator;
     this.errorObserver = new ErrorPrintingTargetEdgeErrorObserver(this.eventHandler);
     this.loadingPhaseThreads = loadingPhaseThreads;
-    this.labelVisitor = new LabelVisitor(targetProvider, dependencyFilter);
+    this.labelVisitor = new LabelVisitor(targetProvider, dependencyFilter, queryIdForDebugging);
   }
 
   @Override

src/main/java/com/google/devtools/build/lib/query2/query/GraphlessBlazeQueryEnvironment.java

@@ -84,6 +84,7 @@ public class GraphlessBlazeQueryEnvironment extends AbstractBlazeQueryEnvironmen
   private final QueryTransitivePackagePreloader queryTransitivePackagePreloader;
   private final TargetProvider targetProvider;
   private final CachingPackageLocator cachingPackageLocator;
+  private final String queryIdForDebugging;
   private final ErrorPrintingTargetEdgeErrorObserver errorObserver;
   private final LabelVisitor labelVisitor;
   protected final int loadingPhaseThreads;
@@ -116,16 +117,18 @@ public GraphlessBlazeQueryEnvironment(
       Predicate<Label> labelFilter,
       ExtendedEventHandler eventHandler,
       Set<Setting> settings,
-      Iterable<QueryFunction> extraFunctions) {
+      Iterable<QueryFunction> extraFunctions,
+      String queryIdForDebugging) {
     super(keepGoing, strictScope, labelFilter, eventHandler, settings, extraFunctions);
     this.targetPatternPreloader = targetPatternPreloader;
     this.relativeWorkingDirectory = relativeWorkingDirectory;
     this.queryTransitivePackagePreloader = queryTransitivePackagePreloader;
     this.targetProvider = targetProvider;
     this.cachingPackageLocator = cachingPackageLocator;
+    this.queryIdForDebugging = queryIdForDebugging;
     this.errorObserver = new ErrorPrintingTargetEdgeErrorObserver(this.eventHandler);
     this.loadingPhaseThreads = loadingPhaseThreads;
-    this.labelVisitor = new LabelVisitor(targetProvider, dependencyFilter);
+    this.labelVisitor = new LabelVisitor(targetProvider, dependencyFilter, queryIdForDebugging);
   }
 
   @Override
@@ -236,7 +239,7 @@ public void deps(
     }
     Set<Target> result = Sets.newConcurrentHashSet();
     try (SilentCloseable closeable = Profiler.instance().profile("syncUncached")) {
-      new LabelVisitor(targetProvider, dependencyFilter)
+      new LabelVisitor(targetProvider, dependencyFilter, queryIdForDebugging)
           .syncUncached(
               eventHandler,
               from,

src/main/java/com/google/devtools/build/lib/query2/query/LabelVisitor.java

@@ -67,6 +67,7 @@
  */
 final class LabelVisitor {
   private static final VisitationAttributes NONE = new VisitationAttributes(ImmutableSet.of(), 0);
+  private final String debugString;
 
   /** Attributes of a visitation which determine whether it is up-to-date or not. */
   private static class VisitationAttributes {
@@ -152,10 +153,12 @@ boolean current(VisitationAttributes lastVisitation) {
    * @param targetProvider how to resolve labels to targets
    * @param edgeFilter which edges may be traversed
    */
-  public LabelVisitor(TargetProvider targetProvider, DependencyFilter edgeFilter) {
+  public LabelVisitor(
+      TargetProvider targetProvider, DependencyFilter edgeFilter, String debugString) {
     this.targetProvider = targetProvider;
     this.lastVisitation = NONE;
     this.edgeFilter = edgeFilter;
+    this.debugString = debugString;
   }
 
   void syncWithVisitor(
@@ -202,7 +205,8 @@ private boolean redoVisitation(
       throws InterruptedException {
     visitedTargets.clear();
 
-    Visitor visitor = new Visitor(eventHandler, keepGoing, parallelThreads, maxDepth, observer);
+    Visitor visitor =
+        new Visitor(eventHandler, keepGoing, parallelThreads, maxDepth, observer, debugString);
 
     Throwable uncaught = null;
     boolean result;
@@ -224,7 +228,7 @@ public boolean hasVisited(Label target) {
   }
 
   private class Visitor {
-    private static final String THREAD_NAME = "LabelVisitor";
+    private static final String THREAD_NAME = "LabelVisitor-";
 
     private final ExecutorService executorService;
     private final QuiescingExecutor executor;
@@ -240,15 +244,19 @@ private class Visitor {
         boolean keepGoing,
         int parallelThreads,
         int maxDepth,
-        TargetEdgeObserver observer) {
+        TargetEdgeObserver observer,
+        String debugString) {
       if (parallelThreads > 1) {
-        this.executorService = NamedForkJoinPool.newNamedPool(THREAD_NAME, parallelThreads);
+        this.executorService =
+            NamedForkJoinPool.newNamedPool(THREAD_NAME + debugString, parallelThreads);
       } else {
         // ForkJoinPool has a bug where it deadlocks with parallelism=1, so use a
         // SingleThreadExecutor instead.
         this.executorService =
             Executors.newSingleThreadExecutor(
-                new ThreadFactoryBuilder().setNameFormat(THREAD_NAME + " %d").build());
+                new ThreadFactoryBuilder()
+                    .setNameFormat(THREAD_NAME + debugString + " %d")
+                    .build());
       }
       this.executor =
           AbstractQueueVisitor.createWithExecutorService(

src/main/java/com/google/devtools/build/lib/rules/genquery/GenQuery.java

@@ -362,8 +362,8 @@ private GenQueryResult doQuery(
       }
       AbstractBlazeQueryEnvironment<Target> queryEnvironment =
           QUERY_ENVIRONMENT_FACTORY.create(
-              /*transitivePackageLoader=*/ null,
-              /* graphFactory= */ null,
+              /*queryTransitivePackagePreloader=*/ null,
+              /*graphFactory=*/ null,
               packageProvider,
               packageProvider,
               preloader,
@@ -382,7 +382,8 @@ private GenQueryResult doQuery(
               /*extraFunctions=*/ ImmutableList.of(),
               /*packagePath=*/ null,
               /*blockUniverseEvaluationErrors=*/ false,
-              /*useGraphlessQuery=*/ graphlessQuery);
+              /*useGraphlessQuery=*/ graphlessQuery,
+              "genquery for " + ruleContext.getLabel());
       QueryExpression expr = QueryExpression.parse(query, queryEnvironment);
       formatter.verifyCompatible(queryEnvironment, expr);
       targets =

src/main/java/com/google/devtools/build/lib/runtime/commands/QueryEnvironmentBasedCommand.java

@@ -277,7 +277,8 @@ public static AbstractBlazeQueryEnvironment<Target> newQueryEnvironment(
             env.getRuntime().getQueryFunctions(),
             env.getPackageManager().getPackagePath(),
             /*blockUniverseEvaluationErrors=*/ false,
-            useGraphlessQuery);
+            useGraphlessQuery,
+            env.getCommandId().toString());
   }
 
   private static BlazeCommandResult reportAndCreateInterruptResult(

src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java

@@ -2566,40 +2566,38 @@ class SkyframePackageLoader {
      */
     Package getPackage(ExtendedEventHandler eventHandler, PackageIdentifier pkgName)
         throws InterruptedException, NoSuchPackageException {
+      SkyKey key = PackageValue.key(pkgName);
+      ImmutableList<SkyKey> keys = ImmutableList.of(key);
+      EvaluationResult<PackageValue> result;
       synchronized (valueLookupLock) {
-        SkyKey key = PackageValue.key(pkgName);
-        // Any call to this method post-loading phase should either be error-free or be in a
-        // keep_going build, since otherwise the build would have failed during loading. Thus
-        // we set keepGoing=true unconditionally.
-        EvaluationResult<PackageValue> result =
-            evaluate(
-                ImmutableList.of(key),
-                /*keepGoing=*/ true,
-                /*numThreads=*/ DEFAULT_THREAD_COUNT,
-                eventHandler);
-        ErrorInfo error = result.getError(key);
-        if (error != null) {
-          if (!error.getCycleInfo().isEmpty()) {
-            reportCycles(eventHandler, result.getError().getCycleInfo(), key);
-            // This can only happen if a package is freshly loaded outside of the target parsing
-            // or loading phase
-            throw new BuildFileContainsErrorsException(
-                pkgName, "Cycle encountered while loading package " + pkgName);
-          }
-          Throwable e = Preconditions.checkNotNull(error.getException(), "%s %s", pkgName, error);
-          // PackageFunction should be catching, swallowing, and rethrowing all transitive
-          // errors as NoSuchPackageExceptions or constructing packages with errors, since we're in
-          // keep_going mode.
-          Throwables.throwIfInstanceOf(e, NoSuchPackageException.class);
-          throw new IllegalStateException(
-              "Unexpected Exception type from PackageValue for '"
-                  + pkgName
-                  + "'' with error: "
-                  + error,
-              e);
+        // Loading a single package shouldn't be too bad to do in keep_going mode even if the build
+        // overall is in nokeep_going mode: the worst that happens is we parse some unnecessary
+        // .bzl files.
+        result =
+            evaluate(keys, /*keepGoing=*/ true, /*numThreads=*/ DEFAULT_THREAD_COUNT, eventHandler);
+      }
+      ErrorInfo error = result.getError(key);
+      if (error != null) {
+        if (!error.getCycleInfo().isEmpty()) {
+          reportCycles(eventHandler, result.getError().getCycleInfo(), key);
+          // This can only happen if a package is freshly loaded outside of the target parsing or
+          // loading phase
+          throw new BuildFileContainsErrorsException(
+              pkgName, "Cycle encountered while loading package " + pkgName);
         }
-        return result.get(key).getPackage();
+        Throwable e = Preconditions.checkNotNull(error.getException(), "%s %s", pkgName, error);
+        // PackageFunction should be catching, swallowing, and rethrowing all transitive errors as
+        // NoSuchPackageExceptions or constructing packages with errors, since we're in keep_going
+        // mode.
+        Throwables.throwIfInstanceOf(e, NoSuchPackageException.class);
+        throw new IllegalStateException(
+            "Unexpected Exception type from PackageValue for '"
+                + pkgName
+                + "'' with error: "
+                + error,
+            e);
       }
+      return result.get(key).getPackage();
     }
 
     /** Returns whether the given package should be consider deleted and thus should be ignored. */

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

@@ -297,8 +297,12 @@ private void performInvalidation() throws InterruptedException {
   }
 
   private void setAndCheckEvaluateState(boolean newValue, Object requestInfo) {
-    Preconditions.checkState(evaluating.getAndSet(newValue) != newValue,
-        "Re-entrant evaluation for request: %s", requestInfo);
+    Preconditions.checkState(
+        evaluating.getAndSet(newValue) != newValue,
+        "Re-entrant evaluation for request: %s (version=%s, current thread=%s)",
+        requestInfo,
+        lastGraphVersion,
+        Thread.currentThread());
   }
 
   @Override

src/test/java/com/google/devtools/build/lib/query2/engine/GraphlessQueryTest.java

@@ -114,7 +114,8 @@ public AbstractBlazeQueryEnvironment<Target> create(
               Iterable<QueryFunction> extraFunctions,
               @Nullable PathPackageLocator packagePath,
               boolean blockUniverseEvaluationErrors,
-              boolean useGraphlessQuery) {
+              boolean useGraphlessQuery,
+              String queryIdForDebugging) {
             return new GraphlessBlazeQueryEnvironment(
                 queryTransitivePackagePreloader,
                 targetProvider,
@@ -127,7 +128,8 @@ public AbstractBlazeQueryEnvironment<Target> create(
                 labelFilter,
                 eventHandler,
                 settings,
-                extraFunctions);
+                extraFunctions,
+                queryIdForDebugging);
           }
         };
       }

src/test/java/com/google/devtools/build/lib/query2/testutil/SkyframeQueryHelper.java

@@ -213,7 +213,8 @@ public AbstractBlazeQueryEnvironment<Target> getQueryEnvironment() {
         getExtraQueryFunctions(),
         pkgManager.getPackagePath(),
         blockUniverseEvaluationErrors,
-        /*useGraphlessQuery=*/ false);
+        /*useGraphlessQuery=*/ false,
+        "debugging string");
   }
 
   protected abstract Iterable<QueryFunction> getExtraQueryFunctions();