diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD index 5a71b5a2fa12cf..d31e9135322c9a 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD @@ -849,6 +849,7 @@ java_library( name = "bzl_compile_value", srcs = ["BzlCompileValue.java"], deps = [ + ":filesystem_keys", ":sky_functions", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization", diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java index f07ba453d4c638..7fbe91d29ea559 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileFunction.java @@ -293,7 +293,7 @@ private static void addSyntaxErrorsToListener( static final class FailedIOException extends Exception { private final Transience transience; - private FailedIOException(IOException cause, Transience transience) { + FailedIOException(IOException cause, Transience transience) { super(cause.getMessage(), cause); this.transience = transience; } diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileValue.java index 3ae4acabf485cc..baf8e825181f23 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileValue.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlCompileValue.java @@ -21,6 +21,7 @@ import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant; import com.google.devtools.build.lib.vfs.Root; +import com.google.devtools.build.lib.vfs.RootedPath; import com.google.devtools.build.skyframe.NotComparableSkyValue; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; @@ -207,6 +208,16 @@ public Label getLabel() { return label; } + /** Returns the dep to register for the underlying .bzl file (if any). */ + @Nullable + public FileKey getBzlFileKey() { + if (label == null) { + // Implies kind == Kind.EMPTY_PRELUDE. + return null; + } + return FileKey.create(RootedPath.toRootedPath(root, label.toPathFragment())); + } + @Override public int hashCode() { return Objects.hash(Key.class, root, label, kind); diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java index e561cb20f6ec57..695fcd3be4b135 100644 --- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java +++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java @@ -62,6 +62,7 @@ import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; import com.google.devtools.build.skyframe.SkyframeLookupResult; +import java.io.IOException; import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashSet; @@ -1499,6 +1500,19 @@ public BzlCompileValue getBzlCompileValue(BzlCompileValue.Key key, Environment e if (value != null) { bzlCompileCache.put(key, value); } + } else { + // The cache hit may have been populated on behalf of a different BzlLoadValue node with + // the same compile key; make sure this node depends on the .bzl file too. + var bzlFileKey = key.getBzlFileKey(); + if (bzlFileKey != null) { + try { + if (env.getValueOrThrow(bzlFileKey, IOException.class) == null) { + return null; + } + } catch (IOException e) { + throw new BzlCompileFunction.FailedIOException(e, Transience.PERSISTENT); + } + } } return value; } diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD index fb14784a473b01..db91c75041458e 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BUILD +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BUILD @@ -2117,6 +2117,7 @@ java_test( "//src/test/java/com/google/devtools/build/lib/analysis/util", "//src/test/java/com/google/devtools/build/lib/bazel/bzlmod:util", "//src/test/java/com/google/devtools/build/lib/testutil:TestConstants", + "//src/test/java/com/google/devtools/build/lib/testutil:TestUtils", "//src/test/java/com/google/devtools/build/skyframe:testutil", "//third_party:guava", "//third_party:jsr305", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java index 4d169b6b175ff5..bc359d1eae0e32 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java @@ -34,10 +34,12 @@ import com.google.devtools.build.lib.runtime.QuiescingExecutorsImpl; import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils; import com.google.devtools.build.lib.testutil.TestConstants; +import com.google.devtools.build.lib.testutil.TestUtils; import com.google.devtools.build.lib.util.io.TimestampGranularityMonitor; import com.google.devtools.build.lib.vfs.DigestHashFunction; import com.google.devtools.build.lib.vfs.FileStatus; import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.ModifiedFileSet; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; import com.google.devtools.build.lib.vfs.Root; @@ -50,6 +52,9 @@ import java.io.IOException; import java.io.InputStream; import java.util.UUID; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.Nullable; import net.starlark.java.eval.StarlarkInt; import org.junit.Before; @@ -1157,9 +1162,86 @@ public void testErrorStatingBzlFileInFileStateFunctionIsPersistent() throws Exce assertThatEvaluationResult(result).hasErrorEntryForKeyThat(key).isNotTransient(); } + @Test + public void bzlCompileCacheHitFromNodeWithOtherKeyKind_registersFileDep() throws Exception { + // Regression test for https://github.com/bazelbuild/bazel/issues/30900: the KeyForBuild and + // KeyForBzlmod variants of the same .bzl share a single entry in BzlLoadFunction's + // bzlCompileCache. A node that gets a cache hit for an entry computed on behalf of the other + // variant must still register a dependency on the .bzl's FileValue; otherwise it isn't + // invalidated when the file changes and keeps serving stale contents. + CustomInMemoryFs fs = (CustomInMemoryFs) fileSystem; + scratch.file("pkg/BUILD"); + scratch.file( + "pkg/foo.bzl", + """ + load(":bar.bzl", "x") + + y = x + """); + Path barBzl = scratch.file("pkg/bar.bzl", "x = 1"); + + // Evaluate the KeyForBuild node on another thread and interrupt the evaluation while it is + // blocked statting bar.bzl. At that point foo.bzl has already been compiled and cached in the + // bzlCompileCache (a .bzl is compiled before its load() deps are requested), and the interrupt + // strands the entry there since it is only released when the owning BzlLoadValue node + // completes. This simulates a .bzl file compiled on behalf of one BzlLoadValue node while a + // node for the other key variant of the same file is in flight. + SkyKey keyForBuild = key("//pkg:foo.bzl"); + fs.pathToBlockOnStat = barBzl; + AtomicBoolean evaluationInterrupted = new AtomicBoolean(false); + Thread evalThread = + new Thread( + () -> { + try { + SkyframeExecutorTestUtils.evaluate( + getSkyframeExecutor(), keyForBuild, /* keepGoing= */ false, reporter); + } catch (InterruptedException e) { + evaluationInterrupted.set(true); + } + }); + evalThread.start(); + assertThat(fs.blockedStatReached.await(TestUtils.WAIT_TIMEOUT_SECONDS, TimeUnit.SECONDS)) + .isTrue(); + evalThread.interrupt(); + evalThread.join(); + assertThat(evaluationInterrupted.get()).isTrue(); + fs.pathToBlockOnStat = null; + fs.blockedStatMayProceed.countDown(); + + // The interrupted evaluation may have committed an error for bar.bzl's FileStateValue (the + // blocked stat throws IOException when the evaluation shuts down). Invalidate it so + // that the next evaluation stats the file afresh. + getSkyframeExecutor() + .invalidateFilesUnderPathForTesting( + reporter, + ModifiedFileSet.builder().modify(PathFragment.create("pkg/bar.bzl")).build(), + Root.fromPath(rootDirectory)); + + // The KeyForBzlmod node gets a bzlCompileCache hit for the entry compiled on behalf of the + // KeyForBuild node above. + SkyKey keyForBzlmod = BzlLoadValue.keyForBzlmod(Label.parseCanonical("//pkg:foo.bzl")); + EvaluationResult result = get(keyForBzlmod); + assertThat(result.get(keyForBzlmod).getModule().getGlobals()) + .containsEntry("y", StarlarkInt.of(1)); + + // Change foo.bzl. The KeyForBzlmod node must pick up the new file contents. + scratch.overwriteFile("pkg/foo.bzl", "y = 2"); + getSkyframeExecutor() + .invalidateFilesUnderPathForTesting( + reporter, + ModifiedFileSet.builder().modify(PathFragment.create("pkg/foo.bzl")).build(), + Root.fromPath(rootDirectory)); + result = get(keyForBzlmod); + assertThat(result.get(keyForBzlmod).getModule().getGlobals()) + .containsEntry("y", StarlarkInt.of(2)); + } + private static class CustomInMemoryFs extends InMemoryFileSystem { @Nullable private Path badPathForStat; @Nullable private Path badPathForRead; + @Nullable private volatile Path pathToBlockOnStat; + private final CountDownLatch blockedStatReached = new CountDownLatch(1); + private final CountDownLatch blockedStatMayProceed = new CountDownLatch(1); CustomInMemoryFs() { super(DigestHashFunction.SHA256); @@ -1170,6 +1252,16 @@ public FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws if (badPathForStat != null && badPathForStat.asFragment().equals(path)) { throw new IOException("bad"); } + Path blockedPath = pathToBlockOnStat; + if (blockedPath != null && blockedPath.asFragment().equals(path)) { + blockedStatReached.countDown(); + try { + blockedStatMayProceed.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new IOException("interrupted"); + } + } return super.statIfFound(path, followSymlinks); }