bep: introduce BuildEventArtifactUploaderFactory

There can be multiple BuildEventTransports active at
the same time and we need to ensure that each transport
gets its own BuildEventArtifactUploader as these transports
might have different lifecycles.

We do that by introducing another level of indirection via
the BuildEventArtifactUploaderFactory. BlazeModules now
register a factory object instead of an uploader.

In addition, the BuildEventArtifactUploader gets a shutdown()
method that allows to free any resources associated with it.

PiperOrigin-RevId: 203752092

5 files changed, 114 insertions, 73 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java b/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java
index 37ac8f6c95..cda7069033 100644
--- a/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildeventservice/BazelBuildEventServiceModuleTest.java
@@ -26,8 +26,8 @@ import com.google.devtools.build.lib.actions.ActionExecutedEvent;
 import com.google.devtools.build.lib.actions.ActionExecutedEvent.ErrorTiming;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
 import com.google.devtools.build.lib.authandtls.AuthAndTLSOptions;
-import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader;
-import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploaderMap;
+import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploaderFactory;
+import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploaderFactoryMap;
 import com.google.devtools.build.lib.buildeventstream.BuildEventProtocolOptions;
 import com.google.devtools.build.lib.buildeventstream.transports.BinaryFormatFileTransport;
 import com.google.devtools.build.lib.buildeventstream.transports.BuildEventStreamOptions;
@@ -138,8 +138,8 @@ public class BazelBuildEventServiceModuleTest {
         commandLineReporter,
         moduleEnvironment,
         clock,
-        new BuildEventArtifactUploaderMap.Builder()
-            .add("", BuildEventArtifactUploader.LOCAL_FILES_UPLOADER)
+        new BuildEventArtifactUploaderFactoryMap.Builder()
+            .add("", BuildEventArtifactUploaderFactory.LOCAL_FILES_UPLOADER_FACTORY)
             .build(),
         reporter,
         /* buildRequestId= */ "foo",
diff --git a/src/test/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploaderFactoryMapTest.java b/src/test/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploaderFactoryMapTest.java
new file mode 100644
index 0000000000..8bf867ae6a
--- /dev/null
+++ b/src/test/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploaderFactoryMapTest.java
@@ -0,0 +1,75 @@
+// 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.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.vfs.Path;
+import java.util.Map;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Tests for {@link BuildEventArtifactUploaderFactoryMap}. */
+@RunWith(JUnit4.class)
+public final class BuildEventArtifactUploaderFactoryMapTest {
+  private BuildEventArtifactUploaderFactoryMap uploaderFactories;
+  private BuildEventArtifactUploaderFactory noConversionUploaderFactory;
+
+  @Before
+  public void setUp() {
+    noConversionUploaderFactory =
+        () ->
+            new BuildEventArtifactUploader() {
+              @Override
+              public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
+                return Futures.immediateFuture(PathConverter.NO_CONVERSION);
+              }
+
+              @Override
+              public void shutdown() {
+                // Intentionally left empty.
+              }
+            };
+    uploaderFactories =
+        new BuildEventArtifactUploaderFactoryMap.Builder()
+            .add("a", BuildEventArtifactUploaderFactory.LOCAL_FILES_UPLOADER_FACTORY)
+            .add("b", noConversionUploaderFactory)
+            .build();
+  }
+
+  @Test
+  public void testEmptyUploaders() throws Exception {
+    BuildEventArtifactUploaderFactoryMap emptyUploader =
+        new BuildEventArtifactUploaderFactoryMap.Builder().build();
+    assertThat(emptyUploader.select(null).create())
+        .isEqualTo(BuildEventArtifactUploader.LOCAL_FILES_UPLOADER);
+  }
+
+  @Test
+  public void testAlphabeticalOrder() {
+    assertThat(uploaderFactories.select(null).create())
+        .isEqualTo(BuildEventArtifactUploader.LOCAL_FILES_UPLOADER);
+  }
+
+  @Test
+  public void testSelectByName() throws Exception {
+    assertThat(uploaderFactories.select("b"))
+        .isEqualTo(noConversionUploaderFactory);
+  }
+}
diff --git a/src/test/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploaderMapTest.java b/src/test/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploaderMapTest.java
deleted file mode 100644
index 848fdf3a30..0000000000
--- a/src/test/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploaderMapTest.java
+++ /dev/null
@@ -1,57 +0,0 @@
-// 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.util.concurrent.Futures;
-import org.junit.Before;
-import org.junit.Test;
-import org.junit.runner.RunWith;
-import org.junit.runners.JUnit4;
-
-/** Tests for {@link BuildEventArtifactUploaderMap}. */
-@RunWith(JUnit4.class)
-public final class BuildEventArtifactUploaderMapTest {
-  private BuildEventArtifactUploaderMap uploader;
-  private BuildEventArtifactUploader noConversionUploader;
-
-  @Before
-  public void setUp() {
-    noConversionUploader = files -> Futures.immediateFuture(PathConverter.NO_CONVERSION);
-    uploader =
-        new BuildEventArtifactUploaderMap.Builder()
-            .add("a", BuildEventArtifactUploader.LOCAL_FILES_UPLOADER)
-            .add("b", noConversionUploader)
-            .build();
-  }
-
-  @Test
-  public void testEmptyUploaders() throws Exception {
-    BuildEventArtifactUploaderMap emptyUploader =
-        new BuildEventArtifactUploaderMap.Builder().build();
-    assertThat(emptyUploader.select(null))
-        .isEqualTo(BuildEventArtifactUploader.LOCAL_FILES_UPLOADER);
-  }
-
-  @Test
-  public void testAlphabeticalOrder() throws Exception {
-    assertThat(uploader.select(null)).isEqualTo(BuildEventArtifactUploader.LOCAL_FILES_UPLOADER);
-  }
-
-  @Test
-  public void testSelectByName() throws Exception {
-    assertThat(uploader.select("b")).isEqualTo(noConversionUploader);
-  }
-}
diff --git a/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BinaryFormatFileTransportTest.java b/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BinaryFormatFileTransportTest.java
index a20345de27..7bc077e8ac 100644
--- a/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BinaryFormatFileTransportTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BinaryFormatFileTransportTest.java
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.buildeventstream.transports;
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader.LOCAL_FILES_UPLOADER;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
 import com.google.common.collect.ImmutableList;
@@ -24,6 +25,7 @@ import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.SettableFuture;
 import com.google.devtools.build.lib.buildeventstream.ArtifactGroupNamer;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent;
+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.buildeventstream.BuildEventArtifactUploader;
 import com.google.devtools.build.lib.buildeventstream.BuildEventContext;
@@ -42,6 +44,7 @@ import java.io.FileInputStream;
 import java.io.InputStream;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.Map;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.locks.LockSupport;
@@ -206,14 +209,20 @@ public class BinaryFormatFileTransportTest {
     BuildEvent event1 = new WithLocalFileEvent(file1);
     BuildEvent event2 = new WithLocalFileEvent(file2);
 
-    BuildEventArtifactUploader uploader =
-        files -> {
-          if (files.containsKey(file1)) {
-            LockSupport.parkNanos(TimeUnit.MILLISECONDS.toNanos(200));
-          }
-          return Futures.immediateFuture(new FileUriPathConverter());
-        };
-
+    BuildEventArtifactUploader uploader = Mockito.spy(new BuildEventArtifactUploader() {
+      @Override
+      public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
+        if (files.containsKey(file1)) {
+          LockSupport.parkNanos(TimeUnit.MILLISECONDS.toNanos(200));
+        }
+        return Futures.immediateFuture(new FileUriPathConverter());
+      }
+
+      @Override
+      public void shutdown() {
+        // Intentionally left empty.
+      }
+    });
     File output = tmp.newFile();
     BinaryFormatFileTransport transport =
         new BinaryFormatFileTransport(output.getAbsolutePath(), defaultOpts, uploader, (e) -> {});
@@ -230,6 +239,8 @@ public class BinaryFormatFileTransportTest {
           .isEqualTo(event2.asStreamProto(null));
       assertThat(in.available()).isEqualTo(0);
     }
+
+    verify(uploader).shutdown();
   }
 
   @Test
@@ -241,7 +252,17 @@ public class BinaryFormatFileTransportTest {
     BuildEvent event = new WithLocalFileEvent(file1);
 
     SettableFuture<PathConverter> upload = SettableFuture.create();
-    BuildEventArtifactUploader uploader = files -> upload;
+    BuildEventArtifactUploader uploader = Mockito.spy(new BuildEventArtifactUploader() {
+      @Override
+      public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
+        return upload;
+      }
+
+      @Override
+      public void shutdown() {
+        // Intentionally left empty.
+      }
+    });
 
     File output = tmp.newFile();
     BinaryFormatFileTransport transport =
@@ -258,6 +279,8 @@ public class BinaryFormatFileTransportTest {
           .isEqualTo(event.asStreamProto(null));
       assertThat(in.available()).isEqualTo(0);
     }
+
+    verify(uploader).shutdown();
   }
 
   private static class WithLocalFileEvent implements BuildEvent {
diff --git a/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BuildEventTransportFactoryTest.java b/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BuildEventTransportFactoryTest.java
index 5494684b3f..fe5f916206 100644
--- a/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BuildEventTransportFactoryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/BuildEventTransportFactoryTest.java
@@ -22,7 +22,7 @@ import com.google.common.collect.FluentIterable;
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.buildeventstream.ArtifactGroupNamer;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent;
-import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploaderMap;
+import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploaderFactoryMap;
 import com.google.devtools.build.lib.buildeventstream.BuildEventContext;
 import com.google.devtools.build.lib.buildeventstream.BuildEventProtocolOptions;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
@@ -86,8 +86,8 @@ public class BuildEventTransportFactoryTest {
     Mockito.validateMockitoUsage();
   }
 
-  private BuildEventArtifactUploaderMap localFilesOnly() {
-    return new BuildEventArtifactUploaderMap.Builder().build();
+  private BuildEventArtifactUploaderFactoryMap localFilesOnly() {
+    return new BuildEventArtifactUploaderFactoryMap.Builder().build();
   }
 
   @Test