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

6 files changed, 210 insertions, 41 deletions

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<PathConverter> completedPathConverter =
-            Futures.immediateFuture(new FileUriPathConverter());
-
-        @Override
-        public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> 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<PathConverter> upload(Map<Path, LocalFile> files) {
+    List<ListenableFuture<PathLookupResult>> 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<Path, PathLookupResult> 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<Path, PathLookupResult> pathLookups;
+
+    private PathConverterImpl(Map<Path, PathLookupResult> 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<ListenableFuture<PathDigestPair>> 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<PathDigestPair> upload =
-            Futures.transform(
-                uploader.uploadBlobAsync(chunker, /*forceUpload=*/false),
-                unused -> new PathDigestPair(file, digest),
-                MoreExecutors.directExecutor());
-        uploads.add(upload);
+      ListenableFuture<Boolean> isDirectoryFuture = uploadExecutor.submit(() -> file.isDirectory());
+      ListenableFuture<PathDigestPair> 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<Void> 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<Path, Digest> pathToDigest;
+    private final Set<Path> skippedPaths;
 
-    PathConverterImpl(String remoteServerInstanceName,
-        List<PathDigestPair> uploads) {
+    PathConverterImpl(String remoteServerInstanceName, List<PathDigestPair> uploads) {
       Preconditions.checkNotNull(uploads);
       this.remoteServerInstanceName = remoteServerInstanceName;
       pathToDigest = new HashMap<>(uploads.size());
+      ImmutableSet.Builder<Path> 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<PathConverter> 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<PathConverter> 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;
@@ -162,6 +163,21 @@ public class ByteStreamBuildEventArtifactUploaderTest {
   }
 
   @Test
+  public void testUploadDirectoryDoesNotCrash() throws Exception {
+    Path dir = fs.getPath("/dir");
+    dir.createDirectoryAndParents();
+    Map<Path, LocalFile> 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
     // error is propagated correctly.