Allow modules to specify additional nodes in the graph to be invalidated and use this functionality to properly invalidate http download nodes if the downloaded zip file doesn't match the specified hash.

This still means that the actual files downloaded may not match, but checking all such files may be too expensive.

This helps with #336 but before that issue can be closed all remote repositories (git, etc.), should have similar functionality added.

--
MOS_MIGRATED_REVID=99317085

1 files changed, 29 insertions, 119 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
index d2f4a069ae..941ddbdaec 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/FilesystemValueChecker.java
@@ -19,8 +19,6 @@ import com.google.common.base.Supplier;
 import com.google.common.base.Suppliers;
 import com.google.common.base.Throwables;
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.Iterables;
 import com.google.common.collect.Range;
 import com.google.common.collect.Sets;
 import com.google.common.util.concurrent.ThreadFactoryBuilder;
@@ -28,12 +26,12 @@ import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.concurrent.ExecutorUtil;
 import com.google.devtools.build.lib.concurrent.Sharder;
 import com.google.devtools.build.lib.concurrent.ThrowableRecordingRunnableWrapper;
+import com.google.devtools.build.lib.skyframe.SkyValueDirtinessChecker.DirtyResult;
 import com.google.devtools.build.lib.util.LoggingUtil;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor;
 import com.google.devtools.build.lib.vfs.BatchStat;
 import com.google.devtools.build.lib.vfs.FileStatusWithDigest;
-import com.google.devtools.build.lib.vfs.RootedPath;
 import com.google.devtools.build.skyframe.Differencer;
 import com.google.devtools.build.skyframe.MemoizingEvaluator;
 import com.google.devtools.build.skyframe.SkyFunctionName;
@@ -65,9 +63,6 @@ class FilesystemValueChecker {
   private static final int DIRTINESS_CHECK_THREADS = 50;
   private static final Logger LOG = Logger.getLogger(FilesystemValueChecker.class.getName());
 
-  private static final Predicate<SkyKey> FILE_STATE_AND_DIRECTORY_LISTING_STATE_FILTER =
-      SkyFunctionName.functionIsIn(ImmutableSet.of(SkyFunctions.FILE_STATE,
-          SkyFunctions.DIRECTORY_LISTING_STATE));
   private static final Predicate<SkyKey> ACTION_FILTER =
       SkyFunctionName.functionIs(SkyFunctions.ACTION_EXECUTION);
 
@@ -94,43 +89,20 @@ class FilesystemValueChecker {
     });
   }
 
-  Iterable<SkyKey> getFilesystemSkyKeys() {
-    return Iterables.filter(valuesSupplier.get().keySet(),
-        FILE_STATE_AND_DIRECTORY_LISTING_STATE_FILTER);
-  }
-
-  Differencer.Diff getDirtyFilesystemSkyKeys() throws InterruptedException {
-    return getDirtyFilesystemValues(getFilesystemSkyKeys());
-  }
-
   /**
-   * Check the given file and directory values for modifications. {@code values} is assumed to only
-   * have {@link FileValue}s and {@link DirectoryListingStateValue}s.
+   * Returns a {@link Differencer.Diff} containing keys that are dirty according to the passed-in
+   * {@param dirtinessChecker}.
    */
-  Differencer.Diff getDirtyFilesystemValues(Iterable<SkyKey> values)
+  Differencer.Diff getDirtyKeys(SkyValueDirtinessChecker dirtinessChecker)
       throws InterruptedException {
-    return getDirtyValues(values, FILE_STATE_AND_DIRECTORY_LISTING_STATE_FILTER,
-        new DirtyChecker() {
-      @Override
-      public DirtyResult check(SkyKey key, SkyValue oldValue, TimestampGranularityMonitor tsgm) {
-        if (key.functionName() == SkyFunctions.FILE_STATE) {
-          return checkFileStateValue((RootedPath) key.argument(), (FileStateValue) oldValue,
-              tsgm);
-        } else if (key.functionName() == SkyFunctions.DIRECTORY_LISTING_STATE) {
-          return checkDirectoryListingStateValue((RootedPath) key.argument(),
-              (DirectoryListingStateValue) oldValue);
-        } else {
-          throw new IllegalStateException("Unexpected key type " + key);
-        }
-      }
-    });
+    return getDirtyValues(valuesSupplier.get().keySet(), dirtinessChecker);
   }
 
   /**
    * Return a collection of action values which have output files that are not in-sync with
    * the on-disk file value (were modified externally).
    */
-  public Collection<SkyKey> getDirtyActionValues(@Nullable final BatchStat batchStatter)
+  Collection<SkyKey> getDirtyActionValues(@Nullable final BatchStat batchStatter)
       throws InterruptedException {
     // CPU-bound (usually) stat() calls, plus a fudge factor.
     LOG.info("Accumulating dirty actions");
@@ -262,10 +234,8 @@ class FilesystemValueChecker {
     return modifiedOutputFilesCounter.get();
   }
 
-  /**
-   * Returns the number of modified output files that occur during the previous build.
-   */
-  public int getNumberOfModifiedOutputFilesDuringPreviousBuild() {
+  /** Returns the number of modified output files that occur during the previous build. */
+  int getNumberOfModifiedOutputFilesDuringPreviousBuild() {
     return modifiedOutputFilesIntraBuildCounter.get();
   }
 
@@ -293,29 +263,31 @@ class FilesystemValueChecker {
     return isDirty;
   }
 
-  private BatchDirtyResult getDirtyValues(Iterable<SkyKey> values,
-                                         Predicate<SkyKey> keyFilter,
-                                         final DirtyChecker checker) throws InterruptedException {
-    ExecutorService executor = Executors.newFixedThreadPool(DIRTINESS_CHECK_THREADS,
-        new ThreadFactoryBuilder().setNameFormat("FileSystem Value Invalidator %d").build());
+  private BatchDirtyResult getDirtyValues(
+      Iterable<SkyKey> values, final SkyValueDirtinessChecker checker) throws InterruptedException {
+    ExecutorService executor =
+        Executors.newFixedThreadPool(
+            DIRTINESS_CHECK_THREADS,
+            new ThreadFactoryBuilder().setNameFormat("FileSystem Value Invalidator %d").build());
 
     final BatchDirtyResult batchResult = new BatchDirtyResult();
     ThrowableRecordingRunnableWrapper wrapper =
         new ThrowableRecordingRunnableWrapper("FilesystemValueChecker#getDirtyValues");
     for (final SkyKey key : values) {
-      Preconditions.checkState(keyFilter.apply(key), key);
       final SkyValue value = valuesSupplier.get().get(key);
-      executor.execute(wrapper.wrap(new Runnable() {
-        @Override
-        public void run() {
-          if (value != null) {
-            DirtyResult result = checker.check(key, value, tsgm);
-            if (result.isDirty()) {
-              batchResult.add(key, result.getNewValue());
-            }
-          }
-        }
-      }));
+      executor.execute(
+          wrapper.wrap(
+              new Runnable() {
+                @Override
+                public void run() {
+                  if (value != null) {
+                    DirtyResult result = checker.maybeCheck(key, value, tsgm);
+                    if (result != null && result.isDirty()) {
+                      batchResult.add(key, result.getNewValue());
+                    }
+                  }
+                }
+              }));
     }
 
     boolean interrupted = ExecutorUtil.interruptibleShutdown(executor);
@@ -326,34 +298,9 @@ class FilesystemValueChecker {
     return batchResult;
   }
 
-  private static DirtyResult checkFileStateValue(RootedPath rootedPath,
-      FileStateValue fileStateValue, TimestampGranularityMonitor tsgm) {
-    try {
-      FileStateValue newValue = FileStateValue.create(rootedPath, tsgm);
-      return newValue.equals(fileStateValue)
-          ? DirtyResult.NOT_DIRTY : DirtyResult.dirtyWithNewValue(newValue);
-    } catch (InconsistentFilesystemException | IOException e) {
-      // TODO(bazel-team): An IOException indicates a failure to get a file digest or a symlink
-      // target, not a missing file. Such a failure really shouldn't happen, so failing early
-      // may be better here.
-      return DirtyResult.DIRTY;
-    }
-  }
-
-  private static DirtyResult checkDirectoryListingStateValue(RootedPath dirRootedPath,
-      DirectoryListingStateValue directoryListingStateValue) {
-    try {
-      DirectoryListingStateValue newValue = DirectoryListingStateValue.create(dirRootedPath);
-      return newValue.equals(directoryListingStateValue)
-          ? DirtyResult.NOT_DIRTY : DirtyResult.dirtyWithNewValue(newValue);
-    } catch (IOException e) {
-      return DirtyResult.DIRTY;
-    }
-  }
-
   /**
-   * Result of a batch call to {@link DirtyChecker#check}. Partitions the dirty values based on
-   * whether we have a new value available for them or not.
+   * Result of a batch call to {@link SkyValueDirtinessChecker#maybeCheck}. Partitions the dirty
+   * values based on whether we have a new value available for them or not.
    */
   private static class BatchDirtyResult implements Differencer.Diff {
 
@@ -381,41 +328,4 @@ class FilesystemValueChecker {
     }
   }
 
-  private static class DirtyResult {
-
-    static final DirtyResult NOT_DIRTY = new DirtyResult(false, null);
-    static final DirtyResult DIRTY = new DirtyResult(true, null);
-
-    private final boolean isDirty;
-    @Nullable private final SkyValue newValue;
-
-    private DirtyResult(boolean isDirty, @Nullable SkyValue newValue) {
-      this.isDirty = isDirty;
-      this.newValue = newValue;
-    }
-
-    boolean isDirty() {
-      return isDirty;
-    }
-
-    /**
-     * If {@code isDirty()}, then either returns the new value for the value or {@code null} if
-     * the new value wasn't computed. In the case where the value is dirty and a new value is
-     * available, then the new value can be injected into the skyframe graph. Otherwise, the value
-     * should simply be invalidated.
-     */
-    @Nullable
-    SkyValue getNewValue() {
-      Preconditions.checkState(isDirty());
-      return newValue;
-    }
-
-    static DirtyResult dirtyWithNewValue(SkyValue newValue) {
-      return new DirtyResult(true, newValue);
-    }
-  }
-
-  private static interface DirtyChecker {
-    DirtyResult check(SkyKey key, SkyValue oldValue, TimestampGranularityMonitor tsgm);
-  }
 }