From 9efbc25845c5ec4e6eece8540079fb33669e39e5 Mon Sep 17 00:00:00 2001 From: tomlu Date: Fri, 10 Aug 2018 11:51:20 -0700 Subject: Do not crash when attempting to upload a tree artifact. These show up as directories. Filter these out and return null from the path converter, which should cause omission of those files from any build events. RELNOTES: None PiperOrigin-RevId: 208244910 --- .../BuildEventArtifactUploader.java | 18 +---- .../LocalFilesArtifactUploader.java | 89 ++++++++++++++++++++++ .../ByteStreamBuildEventArtifactUploader.java | 71 +++++++++++------ .../devtools/build/lib/buildeventstream/BUILD | 1 + .../LocalFilesArtifactUploaderTest.java | 56 ++++++++++++++ .../ByteStreamBuildEventArtifactUploaderTest.java | 16 ++++ 6 files changed, 210 insertions(+), 41 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/buildeventstream/LocalFilesArtifactUploader.java create mode 100644 src/test/java/com/google/devtools/build/lib/buildeventstream/LocalFilesArtifactUploaderTest.java (limited to 'src') diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploader.java b/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploader.java index 284ce6de67..67a53142dd 100644 --- a/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploader.java +++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploader.java @@ -13,30 +13,14 @@ // limitations under the License. package com.google.devtools.build.lib.buildeventstream; -import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile; -import com.google.devtools.build.lib.buildeventstream.PathConverter.FileUriPathConverter; import com.google.devtools.build.lib.vfs.Path; import java.util.Map; /** Uploads artifacts referenced by the Build Event Protocol (BEP). */ public interface BuildEventArtifactUploader { - BuildEventArtifactUploader LOCAL_FILES_UPLOADER = - new BuildEventArtifactUploader() { - private final ListenableFuture completedPathConverter = - Futures.immediateFuture(new FileUriPathConverter()); - - @Override - public ListenableFuture upload(Map files) { - return completedPathConverter; - } - - @Override - public void shutdown() { - // Intentionally left empty. - } - }; + BuildEventArtifactUploader LOCAL_FILES_UPLOADER = new LocalFilesArtifactUploader(); /** * Asynchronously uploads a set of files referenced by the protobuf representation of a {@link diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/LocalFilesArtifactUploader.java b/src/main/java/com/google/devtools/build/lib/buildeventstream/LocalFilesArtifactUploader.java new file mode 100644 index 0000000000..2cbe93d3f7 --- /dev/null +++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/LocalFilesArtifactUploader.java @@ -0,0 +1,89 @@ +// Copyright 2018 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.buildeventstream; + +import com.google.common.collect.ImmutableMap; +import com.google.common.util.concurrent.Futures; +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.ListeningExecutorService; +import com.google.common.util.concurrent.MoreExecutors; +import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile; +import com.google.devtools.build.lib.vfs.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.concurrent.Executors; +import javax.annotation.Nullable; + +/** An uploader that simply turns paths into local file URIs. */ +class LocalFilesArtifactUploader implements BuildEventArtifactUploader { + private final ListeningExecutorService uploadExecutor = + MoreExecutors.listeningDecorator(Executors.newCachedThreadPool()); + + @Override + public ListenableFuture upload(Map files) { + List> lookups = new ArrayList<>(); + for (Path path : files.keySet()) { + lookups.add(uploadExecutor.submit(() -> new PathLookupResult(path, path.isDirectory()))); + } + return Futures.transform( + Futures.allAsList(lookups), + lookupList -> { + ImmutableMap.Builder pathLookups = ImmutableMap.builder(); + for (PathLookupResult lookup : lookupList) { + pathLookups.put(lookup.path, lookup); + } + return new PathConverterImpl(pathLookups.build()); + }, + MoreExecutors.directExecutor()); + } + + @Override + public void shutdown() { + // Intentionally left empty + } + + private static class PathLookupResult { + final Path path; + final boolean isDirectory; + + private PathLookupResult(Path path, boolean isDirectory) { + this.path = path; + this.isDirectory = isDirectory; + } + } + + private static class PathConverterImpl implements PathConverter { + private static final FileUriPathConverter FILE_URI_PATH_CONVERTER = new FileUriPathConverter(); + private final Map pathLookups; + + private PathConverterImpl(Map pathLookups) { + this.pathLookups = pathLookups; + } + + @Nullable + @Override + public String apply(Path path) { + PathLookupResult result = pathLookups.get(path); + if (result == null) { + // We should throw here, the file wasn't declared in BuildEvent#referencedLocalFiles + return null; + } + if (result.isDirectory) { + return null; + } + return FILE_URI_PATH_CONVERTER.apply(result.path); + } + } +} diff --git a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java index d44bfcf12e..2cbe3e6575 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java +++ b/src/main/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploader.java @@ -15,8 +15,10 @@ package com.google.devtools.build.lib.remote; import com.google.common.base.Preconditions; import com.google.common.base.Strings; +import com.google.common.collect.ImmutableSet; import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.ListeningExecutorService; import com.google.common.util.concurrent.MoreExecutors; import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile; import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader; @@ -25,11 +27,12 @@ import com.google.devtools.build.lib.remote.util.DigestUtil; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.remoteexecution.v1test.Digest; import io.grpc.Context; -import java.io.IOException; import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Set; +import java.util.concurrent.Executors; import java.util.concurrent.atomic.AtomicBoolean; import javax.annotation.Nullable; @@ -38,6 +41,8 @@ import javax.annotation.Nullable; */ class ByteStreamBuildEventArtifactUploader implements BuildEventArtifactUploader { + private final ListeningExecutorService uploadExecutor = + MoreExecutors.listeningDecorator(Executors.newCachedThreadPool()); private final Context ctx; private final ByteStreamUploader uploader; private final String remoteServerInstanceName; @@ -63,28 +68,35 @@ class ByteStreamBuildEventArtifactUploader implements BuildEventArtifactUploader } List> uploads = new ArrayList<>(files.size()); - Context prevCtx = ctx.attach(); - try { for (Path file : files.keySet()) { - DigestUtil digestUtil = new DigestUtil(file.getFileSystem().getDigestFunction()); - Digest digest = digestUtil.compute(file); - Chunker chunker = Chunker.builder(digestUtil).setInput(digest, file).build(); - ListenableFuture upload = - Futures.transform( - uploader.uploadBlobAsync(chunker, /*forceUpload=*/false), - unused -> new PathDigestPair(file, digest), - MoreExecutors.directExecutor()); - uploads.add(upload); + ListenableFuture isDirectoryFuture = uploadExecutor.submit(() -> file.isDirectory()); + ListenableFuture digestFuture = + Futures.transformAsync( + isDirectoryFuture, + isDirectory -> { + if (isDirectory) { + return Futures.immediateFuture(new PathDigestPair(file, null)); + } + DigestUtil digestUtil = new DigestUtil(file.getFileSystem().getDigestFunction()); + Digest digest = digestUtil.compute(file); + Chunker chunker = Chunker.builder(digestUtil).setInput(digest, file).build(); + final ListenableFuture upload; + Context prevCtx = ctx.attach(); + try { + upload = uploader.uploadBlobAsync(chunker, /*forceUpload=*/ false); + } finally { + ctx.detach(prevCtx); + } + return Futures.transform( + upload, unused -> new PathDigestPair(file, digest), uploadExecutor); + }, + MoreExecutors.directExecutor()); + uploads.add(digestFuture); } - - return Futures.transform(Futures.allAsList(uploads), - (uploadsDone) -> new PathConverterImpl(remoteServerInstanceName, uploadsDone), - MoreExecutors.directExecutor()); - } catch (IOException e) { - return Futures.immediateFailedFuture(e); - } finally { - ctx.detach(prevCtx); - } + return Futures.transform( + Futures.allAsList(uploads), + pathDigestPairs -> new PathConverterImpl(remoteServerInstanceName, pathDigestPairs), + MoreExecutors.directExecutor()); } @Override @@ -99,15 +111,23 @@ class ByteStreamBuildEventArtifactUploader implements BuildEventArtifactUploader private final String remoteServerInstanceName; private final Map pathToDigest; + private final Set skippedPaths; - PathConverterImpl(String remoteServerInstanceName, - List uploads) { + PathConverterImpl(String remoteServerInstanceName, List uploads) { Preconditions.checkNotNull(uploads); this.remoteServerInstanceName = remoteServerInstanceName; pathToDigest = new HashMap<>(uploads.size()); + ImmutableSet.Builder skippedPaths = ImmutableSet.builder(); for (PathDigestPair pair : uploads) { - pathToDigest.put(pair.getPath(), pair.getDigest()); + Path path = pair.getPath(); + Digest digest = pair.getDigest(); + if (digest != null) { + pathToDigest.put(path, digest); + } else { + skippedPaths.add(path); + } } + this.skippedPaths = skippedPaths.build(); } @Override @@ -115,6 +135,9 @@ class ByteStreamBuildEventArtifactUploader implements BuildEventArtifactUploader Preconditions.checkNotNull(path); Digest digest = pathToDigest.get(path); if (digest == null) { + if (skippedPaths.contains(path)) { + return null; + } // It's a programming error to reference a file that has not been uploaded. throw new IllegalStateException( String.format("Illegal file reference: '%s'", path.getPathString())); diff --git a/src/test/java/com/google/devtools/build/lib/buildeventstream/BUILD b/src/test/java/com/google/devtools/build/lib/buildeventstream/BUILD index 46f5bbbef2..d001bbf85a 100644 --- a/src/test/java/com/google/devtools/build/lib/buildeventstream/BUILD +++ b/src/test/java/com/google/devtools/build/lib/buildeventstream/BUILD @@ -18,6 +18,7 @@ java_test( deps = [ "//src/main/java/com/google/devtools/build/lib/buildeventstream", "//src/main/java/com/google/devtools/build/lib/vfs", + "//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs", "//src/main/java/com/google/devtools/common/options", "//third_party:guava", "//third_party:junit4", diff --git a/src/test/java/com/google/devtools/build/lib/buildeventstream/LocalFilesArtifactUploaderTest.java b/src/test/java/com/google/devtools/build/lib/buildeventstream/LocalFilesArtifactUploaderTest.java new file mode 100644 index 0000000000..5d51abe9a5 --- /dev/null +++ b/src/test/java/com/google/devtools/build/lib/buildeventstream/LocalFilesArtifactUploaderTest.java @@ -0,0 +1,56 @@ +// Copyright 2018 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. +package com.google.devtools.build.lib.buildeventstream; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.ImmutableMap; +import com.google.common.util.concurrent.ListenableFuture; +import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile; +import com.google.devtools.build.lib.buildeventstream.BuildEvent.LocalFile.LocalFileType; +import com.google.devtools.build.lib.vfs.FileSystem; +import com.google.devtools.build.lib.vfs.FileSystemUtils; +import com.google.devtools.build.lib.vfs.Path; +import com.google.devtools.build.lib.vfs.inmemoryfs.InMemoryFileSystem; +import java.nio.charset.StandardCharsets; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Tests for {@link LocalFilesArtifactUploader} */ +@RunWith(JUnit4.class) +public class LocalFilesArtifactUploaderTest { + private final FileSystem fileSystem = new InMemoryFileSystem(); + private final LocalFilesArtifactUploader artifactUploader = new LocalFilesArtifactUploader(); + + @Test + public void testUploadFiles() throws Exception { + Path file = fileSystem.getPath("/test"); + FileSystemUtils.writeContent(file, StandardCharsets.UTF_8, "foo"); + ListenableFuture future = + artifactUploader.upload(ImmutableMap.of(file, new LocalFile(file, LocalFileType.OUTPUT))); + PathConverter pathConverter = future.get(); + assertThat(pathConverter.apply(file)).isEqualTo("file:///test"); + } + + @Test + public void testUploadDirectoryDoesNotCrash() throws Exception { + Path dir = fileSystem.getPath("/test"); + dir.createDirectoryAndParents(); + ListenableFuture future = + artifactUploader.upload(ImmutableMap.of(dir, new LocalFile(dir, LocalFileType.OUTPUT))); + PathConverter pathConverter = future.get(); + assertThat(pathConverter.apply(dir)).isNull(); + } +} diff --git a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java index 75b46c461e..f45e3bca96 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/ByteStreamBuildEventArtifactUploaderTest.java @@ -15,6 +15,7 @@ package com.google.devtools.build.lib.remote; import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.fail; +import static org.mockito.Mockito.mock; import com.google.bytestream.ByteStreamProto.WriteRequest; import com.google.bytestream.ByteStreamProto.WriteResponse; @@ -161,6 +162,21 @@ public class ByteStreamBuildEventArtifactUploaderTest { assertThat(refCntChannel.isShutdown()).isTrue(); } + @Test + public void testUploadDirectoryDoesNotCrash() throws Exception { + Path dir = fs.getPath("/dir"); + dir.createDirectoryAndParents(); + Map filesToUpload = new HashMap<>(); + filesToUpload.put(dir, new LocalFile(dir, LocalFileType.OUTPUT)); + ByteStreamUploader uploader = mock(ByteStreamUploader.class); + ByteStreamBuildEventArtifactUploader artifactUploader = + new ByteStreamBuildEventArtifactUploader( + uploader, "localhost", withEmptyMetadata, "instance"); + PathConverter pathConverter = artifactUploader.upload(filesToUpload).get(); + assertThat(pathConverter.apply(dir)).isNull(); + artifactUploader.shutdown(); + } + @Test public void someUploadsFail() throws Exception { // Test that if one of multiple file uploads fails, the upload future fails and that the -- cgit v1.2.3