commit 390185b734318a4d6d212582fba89f58ec85d995
author felly <felly@google.com> Thu Oct 11 08:55:08 2018 -0700
committer Copybara-Service <copybara-piper@google.com> Thu Oct 11 08:56:55 2018 -0700
tree 517a0992a8a6e5060b8b7a83c70327d3c9936124
parent 804406f5cea9e3c5b93a11b92d64ecdded192fe6

If include scanning ends up throwing, shut down the corresponding parallel visitation. Otherwise, we are in the strange situation where the action is complete, but work done as part of its include scanning continues on in async threads.

PiperOrigin-RevId: 216703648

src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java

diff --git a/src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java b/src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java
index 69ab15e..3333eb9 100644
--- a/src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java
+++ b/src/main/java/com/google/devtools/build/lib/includescanning/LegacyIncludeScanner.java
@@ -501,66 +501,51 @@
     Set<ArtifactWithInclusionContext> visitedInclusions = Sets.newConcurrentHashSet();
 
     IncludeVisitor visitor = new IncludeVisitor(includeScanningHeaderData.getModularHeaders());
-    // Process cmd line includes, if specified.
-    if (mainSource != null && !cmdlineIncludes.isEmpty()) {
-      visitor.processCmdlineIncludes(
-          mainSource,
-          cmdlineIncludes,
-          grepIncludes,
-          includes,
-          includeScanningHeaderData.getPathToLegalOutputArtifact(),
-          actionExecutionMetadata,
-          actionExecutionContext,
-          visitedInclusions);
-      visitor.sync();
-    }
 
-    visitor.processBulkAsync(
-        sources,
-        includes,
-        includeScanningHeaderData.getPathToLegalOutputArtifact(),
-        actionExecutionMetadata,
-        actionExecutionContext,
-        visitedInclusions,
-        grepIncludes);
-    visitor.sync();
+    try {
+      // Process cmd line includes, if specified.
+      if (mainSource != null && !cmdlineIncludes.isEmpty()) {
+        visitor.processCmdlineIncludes(
+            mainSource,
+            cmdlineIncludes,
+            grepIncludes,
+            includes,
+            includeScanningHeaderData.getPathToLegalOutputArtifact(),
+            actionExecutionMetadata,
+            actionExecutionContext,
+            visitedInclusions);
+        visitor.sync();
+      }
 
-    // Process include hints
-    // TODO(ulfjack): Make this code go away. Use the new hinted inclusions instead.
-    Hints hints = parser.getHints();
-    if (hints != null) {
-      // Follow "path" hints.
       visitor.processBulkAsync(
-          pathHints.build(),
+          sources,
           includes,
           includeScanningHeaderData.getPathToLegalOutputArtifact(),
           actionExecutionMetadata,
           actionExecutionContext,
           visitedInclusions,
           grepIncludes);
-      // Follow "file" hints for the primary sources.
-      for (Artifact source : sources) {
-        visitor.processFileLevelHintsAsync(
-            hints,
-            source,
+      visitor.sync();
+
+      // Process include hints
+      // TODO(ulfjack): Make this code go away. Use the new hinted inclusions instead.
+      Hints hints = parser.getHints();
+      if (hints != null) {
+        // Follow "path" hints.
+        visitor.processBulkAsync(
+            pathHints.build(),
             includes,
             includeScanningHeaderData.getPathToLegalOutputArtifact(),
             actionExecutionMetadata,
             actionExecutionContext,
             visitedInclusions,
             grepIncludes);
-      }
-      visitor.sync();
-
-      // Follow "file" hints for all included headers, transitively.
-      Set<Artifact> frontier = includes;
-      while (!frontier.isEmpty()) {
-        Set<Artifact> adjacent = Sets.newConcurrentHashSet();
-        for (Artifact include : frontier) {
+        // Follow "file" hints for the primary sources.
+        for (Artifact source : sources) {
           visitor.processFileLevelHintsAsync(
               hints,
-              include,
-              adjacent,
+              source,
+              includes,
               includeScanningHeaderData.getPathToLegalOutputArtifact(),
               actionExecutionMetadata,
               actionExecutionContext,
@@ -568,14 +553,36 @@
               grepIncludes);
         }
         visitor.sync();
-        // Keep novel nodes as the next frontier.
-        for (Iterator<Artifact> iter = adjacent.iterator(); iter.hasNext(); ) {
-          if (!includes.add(iter.next())) {
-            iter.remove();
+
+        // Follow "file" hints for all included headers, transitively.
+        Set<Artifact> frontier = includes;
+        while (!frontier.isEmpty()) {
+          Set<Artifact> adjacent = Sets.newConcurrentHashSet();
+          for (Artifact include : frontier) {
+            visitor.processFileLevelHintsAsync(
+                hints,
+                include,
+                adjacent,
+                includeScanningHeaderData.getPathToLegalOutputArtifact(),
+                actionExecutionMetadata,
+                actionExecutionContext,
+                visitedInclusions,
+                grepIncludes);
           }
+          visitor.sync();
+          // Keep novel nodes as the next frontier.
+          for (Iterator<Artifact> iter = adjacent.iterator(); iter.hasNext(); ) {
+            if (!includes.add(iter.next())) {
+              iter.remove();
+            }
+          }
+          frontier = adjacent;
         }
-        frontier = adjacent;
       }
+    } catch (IOException | InterruptedException | ExecException | MissingDepException e) {
+      // Careful: Do not leak visitation threads if we have an exception in the initial thread.
+      visitor.sync();
+      throw e;
     }
   }