fix more races in pubsub tests

pongad · pongad · commit 9a308a209f08 · 2016-12-12T17:34:52.000+11:00
Previously BlockingProcessStreamReader has a terminate() method,
used to tell the Reader to stop reading from the emulator process.

This causes an inter-process race.
If the Reader stops before reading emulator's output,
the emulator process will hang as it tries to write to stdout/stderr
as there's no one to read from the other side of the pipe.

Since there is no way to safely stop the Reader,
this commit deletes the method and its associated test.

Additionally, the timeout for LocalSystemTest is increased to 3 minutes,
since the emulator, somehow, consistently takes just longer than a
minute to shut down.
diff --git a/google-cloud-core/src/main/java/com/google/cloud/testing/BaseEmulatorHelper.java b/google-cloud-core/src/main/java/com/google/cloud/testing/BaseEmulatorHelper.java
@@ -114,15 +114,15 @@ protected final void startProcess(String blockUntilOutput)
    * and stop any possible thread listening for its output.
    */
   protected final int waitForProcess(Duration timeout) throws IOException, InterruptedException, TimeoutException {
-    if (blockingProcessReader != null) {
-      blockingProcessReader.terminate();
-      blockingProcessReader = null;
-    }
     if (activeRunner != null) {
       int exitCode = activeRunner.waitFor(timeout);
       activeRunner = null;
       return exitCode;
     }
+    if (blockingProcessReader != null) {
+      blockingProcessReader.join();
+      blockingProcessReader = null;
+    }
     return 0;
   }
 
diff --git a/google-cloud-core/src/main/java/com/google/cloud/testing/BlockingProcessStreamReader.java b/google-cloud-core/src/main/java/com/google/cloud/testing/BlockingProcessStreamReader.java
@@ -61,10 +61,6 @@ private BlockingProcessStreamReader(String emulator, InputStream stream, String
     }
   }
 
-  void terminate() throws IOException {
-    interrupt();
-  }
-
   @Override
   public void run() {
     String previousLine = "";
@@ -79,9 +75,7 @@ public void run() {
         processLogLine(previousLine, nextLine);
       }
     } catch (IOException e) {
-      if (!isInterrupted()) {
-        e.printStackTrace(System.err);
-      }
+      e.printStackTrace(System.err);
     }
     processLogLine(previousLine, firstNonNull(nextLine, ""));
     writeLog();
diff --git a/google-cloud-core/src/test/java/com/google/cloud/testing/BlockingProcessStreamReaderTest.java b/google-cloud-core/src/test/java/com/google/cloud/testing/BlockingProcessStreamReaderTest.java
@@ -74,15 +74,6 @@ Multimap<Level, String> getLogs() {
     }
   }
 
-  @Test
-  public void testBlockUntil() throws IOException {
-    InputStream stream = new ByteArrayInputStream(OUTPUT.getBytes(Charsets.UTF_8));
-    BlockingProcessStreamReader thread =
-        BlockingProcessStreamReader.start("emulator", stream, BLOCK_UNTIL, null);
-    thread.terminate();
-    stream.close();
-  }
-
   @Test
   public void testForwardLogEntry() throws IOException, InterruptedException {
     TestLogger logger = new TestLogger();
diff --git a/google-cloud-pubsub/src/test/java/com/google/cloud/pubsub/LocalSystemTest.java b/google-cloud-pubsub/src/test/java/com/google/cloud/pubsub/LocalSystemTest.java
@@ -50,6 +50,6 @@ public static void startServer() throws IOException, InterruptedException {
   public static void stopServer() throws Exception {
     pubsub.close();
     pubsubHelper.reset();
-    pubsubHelper.stop(Duration.standardMinutes(1));
+    pubsubHelper.stop(Duration.standardMinutes(3));
   }
 }

Original file line number	Diff line number	Diff line change
`@@ -61,10 +61,6 @@ private BlockingProcessStreamReader(String emulator, InputStream stream, String`
`61`	`61`	`}`
`62`	`62`	`}`
`63`	`63`
`64`		`- void terminate() throws IOException {`
`65`		`- interrupt();`
`66`		`- }`
`67`		`-`
`68`	`64`	`@Override`
`69`	`65`	`public void run() {`
`70`	`66`	`String previousLine = "";`
`@@ -79,9 +75,7 @@ public void run() {`
`79`	`75`	`processLogLine(previousLine, nextLine);`
`80`	`76`	`}`
`81`	`77`	`} catch (IOException e) {`
`82`		`- if (!isInterrupted()) {`
`83`		`- e.printStackTrace(System.err);`
`84`		`- }`
	`78`	`+ e.printStackTrace(System.err);`
`85`	`79`	`}`
`86`	`80`	`processLogLine(previousLine, firstNonNull(nextLine, ""));`
`87`	`81`	`writeLog();`
Original file line number	Diff line number	Diff line change
`@@ -50,6 +50,6 @@ public static void startServer() throws IOException, InterruptedException {`
`50`	`50`	`public static void stopServer() throws Exception {`
`51`	`51`	`pubsub.close();`
`52`	`52`	`pubsubHelper.reset();`
`53`		`- pubsubHelper.stop(Duration.standardMinutes(1));`
	`53`	`+ pubsubHelper.stop(Duration.standardMinutes(3));`
`54`	`54`	`}`
`55`	`55`	`}`