From 4693492fda14d47afc100ecc4ef877e3ece94859 Mon Sep 17 00:00:00 2001
From: Kun Zhang <zhangkun83@users.noreply.github.com>
Date: Thu, 29 Dec 2016 11:20:49 -0800
Subject: [PATCH] style: fix styles and error-prones (#2560)

"Because of spurious wakeups, wait() must always be called in a loop".
Got rid of wait().

"If you return or throw from a finally, then values returned or thrown from the try-catch block will be ignored. Consider using try-with-resources instead."
Ignored the exception thrown from finally.
---
 .../io/grpc/PickFirstLoadBalancer2Test.java   |  2 +-
 .../util/RoundRobinLoadBalancer2Test.java     |  4 +-
 .../okhttp/OkHttpClientTransportTest.java     | 57 ++++++++++---------
 3 files changed, 31 insertions(+), 32 deletions(-)

diff --git a/core/src/test/java/io/grpc/PickFirstLoadBalancer2Test.java b/core/src/test/java/io/grpc/PickFirstLoadBalancer2Test.java
index ef4401d298..b92b2e0cef 100644
--- a/core/src/test/java/io/grpc/PickFirstLoadBalancer2Test.java
+++ b/core/src/test/java/io/grpc/PickFirstLoadBalancer2Test.java
@@ -72,7 +72,7 @@ public class PickFirstLoadBalancer2Test {
   private List<ResolvedServerInfoGroup> servers = Lists.newArrayList();
   private List<SocketAddress> socketAddresses = Lists.newArrayList();
 
-  private static Attributes.Key<String> FOO = Attributes.Key.of("foo");
+  private static final Attributes.Key<String> FOO = Attributes.Key.of("foo");
   private Attributes affinity = Attributes.newBuilder().set(FOO, "bar").build();
 
   @Captor
diff --git a/core/src/test/java/io/grpc/util/RoundRobinLoadBalancer2Test.java b/core/src/test/java/io/grpc/util/RoundRobinLoadBalancer2Test.java
index fe12e1ba90..cc945c899a 100644
--- a/core/src/test/java/io/grpc/util/RoundRobinLoadBalancer2Test.java
+++ b/core/src/test/java/io/grpc/util/RoundRobinLoadBalancer2Test.java
@@ -88,15 +88,13 @@ public class RoundRobinLoadBalancer2Test {
   private RoundRobinLoadBalancer loadBalancer;
   private Map<ResolvedServerInfoGroup, EquivalentAddressGroup> servers = Maps.newHashMap();
   private Map<EquivalentAddressGroup, Subchannel> subchannels = Maps.newLinkedHashMap();
-  private static Attributes.Key<String> MAJOR_KEY = Attributes.Key.of("major-key");
+  private static final Attributes.Key<String> MAJOR_KEY = Attributes.Key.of("major-key");
   private Attributes affinity = Attributes.newBuilder().set(MAJOR_KEY, "I got the keys").build();
 
   @Captor
   private ArgumentCaptor<Picker> pickerCaptor;
   @Captor
   private ArgumentCaptor<EquivalentAddressGroup> eagCaptor;
-  @Captor
-  private ArgumentCaptor<Attributes> attrsCaptor;
   @Mock
   private Helper mockHelper;
   @Mock
diff --git a/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java b/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java
index 2a78f68c01..45ca68689f 100644
--- a/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java
+++ b/okhttp/src/test/java/io/grpc/okhttp/OkHttpClientTransportTest.java
@@ -112,6 +112,7 @@ import java.util.List;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.LinkedBlockingQueue;
 import java.util.concurrent.TimeUnit;
 
 import javax.annotation.Nullable;
@@ -1450,10 +1451,15 @@ public class OkHttpClientTransportTest {
   }
 
   private static class MockFrameReader implements FrameReader {
-    CountDownLatch closed = new CountDownLatch(1);
-    boolean throwExceptionForNextFrame;
-    boolean returnFalseInNextFrame;
-    boolean throwErrorForNextFrame;
+    final CountDownLatch closed = new CountDownLatch(1);
+
+    enum Result {
+      THROW_EXCEPTION,
+      RETURN_FALSE,
+      THROW_ERROR
+    }
+
+    final LinkedBlockingQueue<Result> nextResults = new LinkedBlockingQueue<Result>();
 
     @Override
     public void close() throws IOException {
@@ -1472,41 +1478,36 @@ public class OkHttpClientTransportTest {
     }
 
     @Override
-    public synchronized boolean nextFrame(Handler handler) throws IOException {
-      if (throwErrorForNextFrame) {
-        throw new Error(ERROR_MESSAGE);
-      }
-      if (throwExceptionForNextFrame) {
-        throw new IOException(NETWORK_ISSUE_MESSAGE);
-      }
-      if (returnFalseInNextFrame) {
-        return false;
-      }
+    public boolean nextFrame(Handler handler) throws IOException {
+      Result result;
       try {
-        wait();
+        result = nextResults.take();
       } catch (InterruptedException e) {
         Thread.currentThread().interrupt();
         throw new IOException(e);
       }
-      if (throwExceptionForNextFrame) {
-        throw new IOException(NETWORK_ISSUE_MESSAGE);
+      switch (result) {
+        case THROW_EXCEPTION:
+          throw new IOException(NETWORK_ISSUE_MESSAGE);
+        case RETURN_FALSE:
+          return false;
+        case THROW_ERROR:
+          throw new Error(ERROR_MESSAGE);
+        default:
+          throw new UnsupportedOperationException("unimplemented: " + result);
       }
-      return !returnFalseInNextFrame;
     }
 
-    synchronized void throwIoExceptionForNextFrame() {
-      throwExceptionForNextFrame = true;
-      notifyAll();
+    void throwIoExceptionForNextFrame() {
+      nextResults.add(Result.THROW_EXCEPTION);
     }
 
-    synchronized void throwErrorForNextFrame() {
-      throwErrorForNextFrame = true;
-      notifyAll();
+    void throwErrorForNextFrame() {
+      nextResults.add(Result.THROW_ERROR);
     }
 
-    synchronized void nextFrameAtEndOfStream() {
-      returnFalseInNextFrame = true;
-      notifyAll();
+    void nextFrameAtEndOfStream() {
+      nextResults.add(Result.RETURN_FALSE);
     }
 
     @Override
@@ -1574,7 +1575,7 @@ public class OkHttpClientTransportTest {
         try {
           message.close();
         } catch (IOException e) {
-          throw new RuntimeException(e);
+          // Ignore
         }
       }
     }