BEP: Improve the BuildEventTransport interface.

Better specify the BuildEventTransport interface. Besides clarifying threading
and blocking issues, this change also clarifies error handling/reporting.

After several discussions we concluded that the BuildEventTransport interface
should not provide error reporting / handling facilities, as there is not much
bazel could do with this information. Instead, a transport may decide for
itself if an error is fatal and abort the build or if an error should be logged
to the user's terminal or if it should be ignored.

Furthermore, changing the close() method lays the groundwork for an upcoming
change that will report the transport shutdown status to the user command
line.
PiperOrigin-RevId: 152408938

3 files changed, 89 insertions, 7 deletions

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 359f59ed93..1697bb9d1f 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
@@ -15,6 +15,8 @@
 package com.google.devtools.build.lib.buildeventstream.transports;
 
 import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.when;
 
 import com.google.devtools.build.lib.buildeventstream.BuildEvent;
@@ -26,8 +28,8 @@ import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.Tar
 import com.google.devtools.build.lib.buildeventstream.PathConverter;
 import java.io.File;
 import java.io.FileInputStream;
-import java.io.IOException;
 import java.io.InputStream;
+import java.util.concurrent.Future;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Rule;
@@ -61,7 +63,7 @@ public class BinaryFormatFileTransportTest {
   }
 
   @Test
-  public void testCreatesFileAndWritesProtoTextFormat() throws IOException {
+  public void testCreatesFileAndWritesProtoBinaryFormat() throws Exception {
     File output = tmp.newFile();
 
     BuildEventStreamProtos.BuildEvent started =
@@ -85,7 +87,7 @@ public class BinaryFormatFileTransportTest {
     when(buildEvent.asStreamProto(Matchers.<BuildEventConverters>any())).thenReturn(completed);
     transport.sendBuildEvent(buildEvent);
 
-    transport.close();
+    transport.close().get();
     try (InputStream in = new FileInputStream(output)) {
       assertThat(BuildEventStreamProtos.BuildEvent.parseDelimitedFrom(in)).isEqualTo(started);
       assertThat(BuildEventStreamProtos.BuildEvent.parseDelimitedFrom(in)).isEqualTo(progress);
@@ -93,4 +95,81 @@ public class BinaryFormatFileTransportTest {
       assertThat(in.available()).isEqualTo(0);
     }
   }
+
+  @Test
+  public void testFileDoesNotExist() throws Exception {
+    // Get a file that doesn't exist by creating a new file and immediately deleting it.
+    File output = tmp.newFile();
+    String path = output.getAbsolutePath();
+    assertTrue(output.delete());
+
+    BuildEventStreamProtos.BuildEvent started =
+        BuildEventStreamProtos.BuildEvent.newBuilder()
+            .setStarted(BuildStarted.newBuilder().setCommand("build"))
+            .build();
+    when(buildEvent.asStreamProto(Matchers.<BuildEventConverters>any())).thenReturn(started);
+    BinaryFormatFileTransport transport = new BinaryFormatFileTransport(path, pathConverter);
+    transport.sendBuildEvent(buildEvent);
+
+    transport.close().get();
+    try (InputStream in = new FileInputStream(output)) {
+      assertThat(BuildEventStreamProtos.BuildEvent.parseDelimitedFrom(in)).isEqualTo(started);
+      assertThat(in.available()).isEqualTo(0);
+    }
+  }
+
+  @Test
+  public void testWriteWhenFileClosed() throws Exception {
+    File output = tmp.newFile();
+
+    BuildEventStreamProtos.BuildEvent started =
+        BuildEventStreamProtos.BuildEvent.newBuilder()
+            .setStarted(BuildStarted.newBuilder().setCommand("build"))
+            .build();
+    when(buildEvent.asStreamProto(Matchers.<BuildEventConverters>any())).thenReturn(started);
+
+    BinaryFormatFileTransport transport =
+        new BinaryFormatFileTransport(output.getAbsolutePath(), pathConverter);
+
+    // Close the file.
+    transport.ch.close();
+    assertFalse(transport.ch.isOpen());
+
+    // This should not throw an exception.
+    transport.sendBuildEvent(buildEvent);
+    transport.close().get();
+
+    // Also, nothing should have been written to the file
+    try (InputStream in = new FileInputStream(output)) {
+      assertThat(in.available()).isEqualTo(0);
+    }
+  }
+
+  @Test
+  public void testWriteWhenTransportClosed() throws Exception {
+    File output = tmp.newFile();
+
+    BuildEventStreamProtos.BuildEvent started =
+        BuildEventStreamProtos.BuildEvent.newBuilder()
+            .setStarted(BuildStarted.newBuilder().setCommand("build"))
+            .build();
+    when(buildEvent.asStreamProto(Matchers.<BuildEventConverters>any())).thenReturn(started);
+
+    BinaryFormatFileTransport transport =
+        new BinaryFormatFileTransport(output.getAbsolutePath(), pathConverter);
+
+    transport.sendBuildEvent(buildEvent);
+    Future<Void> closeFuture = transport.close();
+    // This should not throw an exception, but also not perform any write.
+    transport.sendBuildEvent(buildEvent);
+
+    closeFuture.get();
+    assertFalse(transport.ch.isOpen());
+
+    // There should have only been one write.
+    try (InputStream in = new FileInputStream(output)) {
+      assertThat(BuildEventStreamProtos.BuildEvent.parseDelimitedFrom(in)).isEqualTo(started);
+      assertThat(in.available()).isEqualTo(0);
+    }
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/TextFormatFileTransportTest.java b/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/TextFormatFileTransportTest.java
index ac62c7476c..7a008fabb0 100644
--- a/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/TextFormatFileTransportTest.java
+++ b/src/test/java/com/google/devtools/build/lib/buildeventstream/transports/TextFormatFileTransportTest.java
@@ -28,7 +28,6 @@ import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos.Tar
 import com.google.devtools.build.lib.buildeventstream.PathConverter;
 import com.google.protobuf.TextFormat;
 import java.io.File;
-import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import org.junit.After;
 import org.junit.Before;
@@ -63,7 +62,7 @@ public class TextFormatFileTransportTest {
   }
 
   @Test
-  public void testCreatesFileAndWritesProtoTextFormat() throws IOException {
+  public void testCreatesFileAndWritesProtoTextFormat() throws Exception {
     File output = tmp.newFile();
 
     BuildEventStreamProtos.BuildEvent started =
@@ -87,7 +86,7 @@ public class TextFormatFileTransportTest {
     when(buildEvent.asStreamProto(Matchers.<BuildEventConverters>any())).thenReturn(completed);
     transport.sendBuildEvent(buildEvent);
 
-    transport.close();
+    transport.close().get();
     String contents =
         trimLines(
             Joiner.on(System.lineSeparator())
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java
index 6d5dd01119..8c6483d66d 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/BuildEventStreamerTest.java
@@ -18,6 +18,7 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 
 import com.google.common.collect.ImmutableSet;
+import com.google.common.util.concurrent.Futures;
 import com.google.devtools.build.lib.buildeventstream.BuildEvent;
 import com.google.devtools.build.lib.buildeventstream.BuildEventConverters;
 import com.google.devtools.build.lib.buildeventstream.BuildEventId;
@@ -31,6 +32,7 @@ import com.google.devtools.build.lib.buildtool.buildevent.BuildCompleteEvent;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
+import java.util.concurrent.Future;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -52,7 +54,9 @@ public class BuildEventStreamerTest {
     }
 
     @Override
-    public void close() {}
+    public Future<Void> close() {
+      return Futures.immediateFuture(null);
+    }
 
     List<BuildEvent> getEvents() {
       return events;