From 130f86ded1ce84f959f0b78c065211902faed546 Mon Sep 17 00:00:00 2001 From: pcloudy Date: Mon, 29 Apr 2019 05:55:23 -0700 Subject: [PATCH] Download stderr/stdout to a temporary FileOutErr On Windows, we cannot delete file without closing the FileOutputStream, but we may need to write to the FileOutErr in fallback execution, so we cannot close the OutputStreams. By using a temporary child FileOutErr, we can safely close and delete the stdout/stderr files of it when the download fails and the original FileOutErr will still be writable. Fixes https://github.com/bazelbuild/bazel/issues/8104 RELNOTES: None PiperOrigin-RevId: 245731397 --- .../lib/remote/AbstractRemoteActionCache.java | 20 +++-- .../AbstractRemoteActionCacheTests.java | 83 +++++++++++++++++++ 2 files changed, 98 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java index da0920bd8cf31e..8d09d7aed962e8 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCache.java @@ -170,7 +170,7 @@ public void onFailure(Throwable t) { * @throws IOException in case of a cache miss or if the remote cache is unavailable. * @throws ExecException in case clean up after a failed download failed. */ - public void download(ActionResult result, Path execRoot, FileOutErr outErr) + public void download(ActionResult result, Path execRoot, FileOutErr origOutErr) throws ExecException, IOException, InterruptedException { ActionResultMetadata metadata = parseActionResultMetadata(result, execRoot); @@ -197,8 +197,12 @@ public void download(ActionResult result, Path execRoot, FileOutErr outErr) IOException downloadException = null; InterruptedException interruptedException = null; + FileOutErr tmpOutErr = null; try { - downloads.addAll(downloadOutErr(result, outErr)); + if (origOutErr != null) { + tmpOutErr = origOutErr.childOutErr(); + } + downloads.addAll(downloadOutErr(result, tmpOutErr)); } catch (IOException e) { downloadException = e; } @@ -228,9 +232,9 @@ public void download(ActionResult result, Path execRoot, FileOutErr outErr) // directories will not be re-created execRoot.getRelative(directory.getPath()).deleteTreesBelow(); } - if (outErr != null) { - outErr.getOutputPath().delete(); - outErr.getErrorPath().delete(); + if (tmpOutErr != null) { + tmpOutErr.clearOut(); + tmpOutErr.clearErr(); } } catch (IOException e) { // If deleting of output files failed, we abort the build with a decent error message as @@ -254,6 +258,12 @@ public void download(ActionResult result, Path execRoot, FileOutErr outErr) throw downloadException; } + if (tmpOutErr != null) { + FileOutErr.dump(tmpOutErr, origOutErr); + tmpOutErr.clearOut(); + tmpOutErr.clearErr(); + } + List symlinksInDirectories = new ArrayList<>(); for (Entry entry : metadata.directories()) { entry.getKey().createDirectoryAndParents(); diff --git a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java index 150289bab3c492..fd63ab88f69a86 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java +++ b/src/test/java/com/google/devtools/build/lib/remote/AbstractRemoteActionCacheTests.java @@ -21,6 +21,7 @@ import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; import build.bazel.remote.execution.v2.Action; import build.bazel.remote.execution.v2.ActionResult; @@ -85,6 +86,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import org.mockito.Mockito; /** Tests for {@link AbstractRemoteActionCache}. */ @RunWith(JUnit4.class) @@ -689,6 +691,87 @@ public void onErrorWaitForRemainingDownloadsToComplete() throws Exception { } } + @Test + public void testDownloadWithStdoutStderrOnSuccess() throws Exception { + // Tests that fetching stdout/stderr as a digest works and that OutErr is still + // writable afterwards. + Path stdout = fs.getPath("/execroot/stdout"); + Path stderr = fs.getPath("/execroot/stderr"); + FileOutErr outErr = new FileOutErr(stdout, stderr); + FileOutErr childOutErr = outErr.childOutErr(); + FileOutErr spyOutErr = Mockito.spy(outErr); + FileOutErr spyChildOutErr = Mockito.spy(childOutErr); + when(spyOutErr.childOutErr()).thenReturn(spyChildOutErr); + + DefaultRemoteActionCache cache = newTestCache(); + Digest digestStdout = cache.addContents("stdout"); + Digest digestStderr = cache.addContents("stderr"); + + ActionResult result = + ActionResult.newBuilder() + .setExitCode(0) + .setStdoutDigest(digestStdout) + .setStderrDigest(digestStderr) + .build(); + + cache.download(result, execRoot, spyOutErr); + + verify(spyOutErr, Mockito.times(2)).childOutErr(); + verify(spyChildOutErr).clearOut(); + verify(spyChildOutErr).clearErr(); + assertThat(outErr.getOutputPath().exists()).isTrue(); + assertThat(outErr.getErrorPath().exists()).isTrue(); + + try { + outErr.getOutputStream().write(0); + outErr.getErrorStream().write(0); + } catch (IOException err) { + throw new AssertionError("outErr should still be writable after download finished.", err); + } + } + + @Test + public void testDownloadWithStdoutStderrOnFailure() throws Exception { + // Test that when downloading stdout/stderr fails the OutErr is still writable + // and empty. + Path stdout = fs.getPath("/execroot/stdout"); + Path stderr = fs.getPath("/execroot/stderr"); + FileOutErr outErr = new FileOutErr(stdout, stderr); + FileOutErr childOutErr = outErr.childOutErr(); + FileOutErr spyOutErr = Mockito.spy(outErr); + FileOutErr spyChildOutErr = Mockito.spy(childOutErr); + when(spyOutErr.childOutErr()).thenReturn(spyChildOutErr); + + DefaultRemoteActionCache cache = newTestCache(); + // Don't add stdout/stderr as a known blob to the remote cache so that downloading it will fail + Digest digestStdout = digestUtil.computeAsUtf8("stdout"); + Digest digestStderr = digestUtil.computeAsUtf8("stderr"); + + ActionResult result = + ActionResult.newBuilder() + .setExitCode(0) + .setStdoutDigest(digestStdout) + .setStderrDigest(digestStderr) + .build(); + try { + cache.download(result, execRoot, spyOutErr); + fail("Expected IOException"); + } catch (IOException e) { + verify(spyOutErr, Mockito.times(2)).childOutErr(); + verify(spyChildOutErr).clearOut(); + verify(spyChildOutErr).clearErr(); + assertThat(outErr.getOutputPath().exists()).isFalse(); + assertThat(outErr.getErrorPath().exists()).isFalse(); + + try { + outErr.getOutputStream().write(0); + outErr.getErrorStream().write(0); + } catch (IOException err) { + throw new AssertionError("outErr should still be writable after download failed.", err); + } + } + } + @Test public void testDownloadMinimalFiles() throws Exception { // Test that injecting the metadata for a remote output file works