Fix race for stream id in OkHttpClientTransport.

buchgr · nmittler · commit f2f120922dd7 · 2015-02-19T21:29:31.000-08:00
When running benchmarks using the okhttp transport with lots of streams per channel we would see the occasional GOAWAY frame with the server logging exceptions of the like "io.netty.handler.codec.http2.Http2Exception: Request stream 575 is behind the next expected stream 583". As quickly identified by @ejona86, there is a race between creating a new stream id and writing the header on the wire. Putting both under the same lock ensures that those two always go together. After this change the errors disappeared. The perf impact should be small as the actual write to the socket doesn't happen within the lock, but only the scheduling of the write.
diff --git a/okhttp/src/main/java/io/grpc/transport/okhttp/OkHttpClientTransport.java b/okhttp/src/main/java/io/grpc/transport/okhttp/OkHttpClientTransport.java
@@ -168,23 +168,29 @@ public class OkHttpClientTransport implements ClientTransport {
   }
 
   @Override
-  public OkHttpClientStream newStream(MethodDescriptor<?, ?> method, Metadata.Headers headers,
-      ClientStreamListener listener) {
+  public OkHttpClientStream newStream(MethodDescriptor<?, ?> method,
+                                      Metadata.Headers headers,
+                                      ClientStreamListener listener) {
     Preconditions.checkNotNull(method, "method");
     Preconditions.checkNotNull(headers, "headers");
     Preconditions.checkNotNull(listener, "listener");
-    OkHttpClientStream clientStream = OkHttpClientStream.newStream(listener,
-        frameWriter, this, outboundFlow);
+
+    OkHttpClientStream clientStream =
+        OkHttpClientStream.newStream(listener, frameWriter, this, outboundFlow);
+
+    String defaultPath = "/" + method.getName();
+    List<Header> requestHeaders =
+        Headers.createRequestHeaders(headers, defaultPath, defaultAuthority);
+
     synchronized (lock) {
       if (goAway) {
         throw new IllegalStateException("Transport not running", goAwayStatus.asRuntimeException());
-      } else {
-        assignStreamId(clientStream);
       }
+      
+      assignStreamId(clientStream);
+      frameWriter.synStream(false, false, clientStream.id(), 0, requestHeaders);
     }
-    String defaultPath = "/" + method.getName();
-    frameWriter.synStream(false, false, clientStream.id(), 0,
-        Headers.createRequestHeaders(headers, defaultPath, defaultAuthority));
+
     return clientStream;
   }
 
