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, 65 insertions, 20 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 3f536325db..284ce6de67 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
@@ -31,6 +31,11 @@ public interface BuildEventArtifactUploader {
         public ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files) {
           return completedPathConverter;
         }
+
+        @Override
+        public void shutdown() {
+          // Intentionally left empty.
+        }
       };
 
   /**
@@ -43,4 +48,9 @@ public interface BuildEventArtifactUploader {
    * as it should appear in the BEP.
    */
   ListenableFuture<PathConverter> upload(Map<Path, LocalFile> files);
+
+  /**
+   * Shutdown any resources associated with the uploader.
+   */
+  void shutdown();
 }
diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploaderFactory.java b/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploaderFactory.java
new file mode 100644
index 0000000000..bee7c40433
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploaderFactory.java
@@ -0,0 +1,28 @@
+// 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.devtools.build.lib.buildeventstream.BuildEventArtifactUploader.LOCAL_FILES_UPLOADER;
+
+/** A factory for {@link BuildEventArtifactUploader}. */
+public interface BuildEventArtifactUploaderFactory {
+
+  BuildEventArtifactUploaderFactory LOCAL_FILES_UPLOADER_FACTORY = () -> LOCAL_FILES_UPLOADER;
+
+  /**
+   * Returns a new instance of a {@link BuildEventArtifactUploader}. The call is responsible for
+   * calling {@link BuildEventArtifactUploader#shutdown()} on the returned instance.
+   */
+  BuildEventArtifactUploader create();
+}
diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploaderMap.java b/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploaderFactoryMap.java
index e9f611ab9a..64c677341d 100644
--- a/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploaderMap.java
+++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/BuildEventArtifactUploaderFactoryMap.java
@@ -21,15 +21,15 @@ import javax.annotation.concurrent.ThreadSafe;
 
 /** Selects between multiple available upload strategies. */
 @ThreadSafe
-public class BuildEventArtifactUploaderMap {
-  private final ImmutableMap<String, BuildEventArtifactUploader> uploaders;
+public class BuildEventArtifactUploaderFactoryMap {
+  private final ImmutableMap<String, BuildEventArtifactUploaderFactory> uploaders;
 
-  private BuildEventArtifactUploaderMap(
-      ImmutableMap<String, BuildEventArtifactUploader> uploaders) {
+  private BuildEventArtifactUploaderFactoryMap(
+      ImmutableMap<String, BuildEventArtifactUploaderFactory> uploaders) {
     this.uploaders = uploaders;
   }
 
-  public BuildEventArtifactUploader select(@Nullable String name) {
+  public BuildEventArtifactUploaderFactory select(@Nullable String name) {
     if (name == null && !uploaders.values().isEmpty()) {
       // TODO(b/110235226): We currently choose the strategy with alphabetically first strategy,
       // which happens to be backwards-compatible; we need to set
@@ -37,23 +37,23 @@ public class BuildEventArtifactUploaderMap {
       // make it an error to pass null.
       return uploaders.values().iterator().next();
     }
-    return uploaders.getOrDefault(name, BuildEventArtifactUploader.LOCAL_FILES_UPLOADER);
+    return uploaders.getOrDefault(
+        name, BuildEventArtifactUploaderFactory.LOCAL_FILES_UPLOADER_FACTORY);
   }
 
-  /** Builder class for {@link BuildEventArtifactUploaderMap}. */
+  /** Builder class for {@link BuildEventArtifactUploaderFactoryMap}. */
   public static class Builder {
-    private final SortedMap<String, BuildEventArtifactUploader> uploaders = new TreeMap<>();
+    private final SortedMap<String, BuildEventArtifactUploaderFactory> uploaders = new TreeMap<>();
 
-    public Builder() {
-    }
+    public Builder() {}
 
-    public Builder add(String name, BuildEventArtifactUploader uploader) {
+    public Builder add(String name, BuildEventArtifactUploaderFactory uploader) {
       uploaders.put(name, uploader);
       return this;
     }
 
-    public BuildEventArtifactUploaderMap build() {
-      return new BuildEventArtifactUploaderMap(ImmutableMap.copyOf(uploaders));
+    public BuildEventArtifactUploaderFactoryMap build() {
+      return new BuildEventArtifactUploaderFactoryMap(ImmutableMap.copyOf(uploaders));
     }
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/transports/BuildEventTransportFactory.java b/src/main/java/com/google/devtools/build/lib/buildeventstream/transports/BuildEventTransportFactory.java
index eef7583451..6cc539418d 100644
--- a/src/main/java/com/google/devtools/build/lib/buildeventstream/transports/BuildEventTransportFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/transports/BuildEventTransportFactory.java
@@ -18,7 +18,7 @@ import static com.google.common.base.Strings.isNullOrEmpty;
 
 import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploader;
-import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploaderMap;
+import com.google.devtools.build.lib.buildeventstream.BuildEventArtifactUploaderFactoryMap;
 import com.google.devtools.build.lib.buildeventstream.BuildEventProtocolOptions;
 import com.google.devtools.build.lib.buildeventstream.BuildEventTransport;
 import com.google.devtools.build.lib.util.AbruptExitException;
@@ -107,14 +107,14 @@ public enum BuildEventTransportFactory {
   public static ImmutableSet<BuildEventTransport> createFromOptions(
       BuildEventStreamOptions options,
       BuildEventProtocolOptions protocolOptions,
-      BuildEventArtifactUploaderMap artifactUploaders,
+      BuildEventArtifactUploaderFactoryMap artifactUploaders,
       Consumer<AbruptExitException> exitFunc)
       throws IOException {
     ImmutableSet.Builder<BuildEventTransport> buildEventTransportsBuilder = ImmutableSet.builder();
     for (BuildEventTransportFactory transportFactory : BuildEventTransportFactory.values()) {
       if (transportFactory.enabled(options)) {
         BuildEventArtifactUploader uploader = transportFactory.usePathConverter(options)
-            ? artifactUploaders.select(protocolOptions.buildEventUploadStrategy)
+            ? artifactUploaders.select(protocolOptions.buildEventUploadStrategy).create()
             : BuildEventArtifactUploader.LOCAL_FILES_UPLOADER;
         buildEventTransportsBuilder.add(
             transportFactory.create(options, protocolOptions, uploader, exitFunc));
diff --git a/src/main/java/com/google/devtools/build/lib/buildeventstream/transports/FileTransport.java b/src/main/java/com/google/devtools/build/lib/buildeventstream/transports/FileTransport.java
index 186a6a5f0c..de29f4b7d4 100644
--- a/src/main/java/com/google/devtools/build/lib/buildeventstream/transports/FileTransport.java
+++ b/src/main/java/com/google/devtools/build/lib/buildeventstream/transports/FileTransport.java
@@ -76,7 +76,7 @@ abstract class FileTransport implements BuildEventTransport {
     this.uploader = uploader;
     this.options = options;
     this.exitFunc = exitFunc;
-    this.writer = new SequentialWriter(path, this::serializeEvent, exitFunc);
+    this.writer = new SequentialWriter(path, this::serializeEvent, exitFunc, uploader);
   }
 
   @ThreadSafe
@@ -90,6 +90,7 @@ abstract class FileTransport implements BuildEventTransport {
     @VisibleForTesting OutputStream out;
     private final Function<BuildEventStreamProtos.BuildEvent, byte[]> serializeFunc;
     private final Consumer<AbruptExitException> exitFunc;
+    private final BuildEventArtifactUploader uploader;
 
     @VisibleForTesting
     final BlockingQueue<ListenableFuture<BuildEventStreamProtos.BuildEvent>> pendingWrites =
@@ -100,7 +101,8 @@ abstract class FileTransport implements BuildEventTransport {
     SequentialWriter(
         String path,
         Function<BuildEventStreamProtos.BuildEvent, byte[]> serializeFunc,
-        Consumer<AbruptExitException> exitFunc) {
+        Consumer<AbruptExitException> exitFunc,
+        BuildEventArtifactUploader uploader) {
       try {
         this.out = new BufferedOutputStream(new FileOutputStream(path));
       } catch (FileNotFoundException e) {
@@ -115,6 +117,7 @@ abstract class FileTransport implements BuildEventTransport {
       this.writerThread = new Thread(this);
       this.serializeFunc = serializeFunc;
       this.exitFunc = exitFunc;
+      this.uploader = uploader;
       writerThread.start();
     }
 
@@ -135,8 +138,12 @@ abstract class FileTransport implements BuildEventTransport {
         logger.log(Level.SEVERE, "Failed to write BEP events to file.", e);
       } finally {
         try {
-          out.flush();
-          out.close();
+          try {
+            out.flush();
+            out.close();
+          } finally {
+            uploader.shutdown();
+          }
         } catch (IOException e) {
           logger.log(Level.SEVERE, "Failed to close BEP file output stream.", e);
         }