From 93e89ba704419183f95211e55c2c5170124901b9 Mon Sep 17 00:00:00 2001
From: Eric Anderson <ejona@google.com>
Date: Mon, 6 Nov 2017 15:03:42 -0800
Subject: netty: Move server transportReady after client preface receipt

This mirrors the behavior of client-side.
---
 .../java/io/grpc/netty/NettyServerHandler.java     | 16 ++++++++++-
 .../java/io/grpc/netty/NettyServerHandlerTest.java | 32 ++++++++++++++++------
 2 files changed, 39 insertions(+), 9 deletions(-)

(limited to 'netty')

diff --git a/netty/src/main/java/io/grpc/netty/NettyServerHandler.java b/netty/src/main/java/io/grpc/netty/NettyServerHandler.java
index 370f889f6..d5432148c 100644
--- a/netty/src/main/java/io/grpc/netty/NettyServerHandler.java
+++ b/netty/src/main/java/io/grpc/netty/NettyServerHandler.java
@@ -106,6 +106,9 @@ class NettyServerHandler extends AbstractNettyHandler {
   private final List<ServerStreamTracer.Factory> streamTracerFactories;
   private final TransportTracer transportTracer;
   private final KeepAliveEnforcer keepAliveEnforcer;
+  /** Incomplete attributes produced by negotiator. */
+  private Attributes negotiationAttributes;
+  /** Completed attributes produced by transportReady. */
   private Attributes attributes;
   private Throwable connectionError;
   private boolean teWarningLogged;
@@ -481,7 +484,7 @@ class NettyServerHandler extends AbstractNettyHandler {
 
   @Override
   public void handleProtocolNegotiationCompleted(Attributes attrs) {
-    attributes = transportListener.transportReady(attrs);
+    negotiationAttributes = attrs;
   }
 
   @VisibleForTesting
@@ -680,6 +683,17 @@ class NettyServerHandler extends AbstractNettyHandler {
   }
 
   private class FrameListener extends Http2FrameAdapter {
+    private boolean firstSettings = true;
+
+    @Override
+    public void onSettingsRead(ChannelHandlerContext ctx, Http2Settings settings) {
+      if (firstSettings) {
+        firstSettings = false;
+        // Delay transportReady until we see the client's HTTP handshake, for coverage with
+        // handshakeTimeout
+        attributes = transportListener.transportReady(negotiationAttributes);
+      }
+    }
 
     @Override
     public int onDataRead(ChannelHandlerContext ctx, int streamId, ByteBuf data, int padding,
diff --git a/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java b/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java
index 707c4e731..cd8ee55e6 100644
--- a/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java
+++ b/netty/src/test/java/io/grpc/netty/NettyServerHandlerTest.java
@@ -89,13 +89,13 @@ import java.util.LinkedList;
 import java.util.List;
 import java.util.Queue;
 import java.util.concurrent.TimeUnit;
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.Timeout;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
 import org.mockito.ArgumentCaptor;
-import org.mockito.Matchers;
 import org.mockito.Mock;
 import org.mockito.MockitoAnnotations;
 import org.mockito.invocation.InvocationOnMock;
@@ -136,7 +136,7 @@ public class NettyServerHandlerTest extends NettyHandlerTestBase<NettyServerHand
   private long maxConnectionAgeGraceInNanos = MAX_CONNECTION_AGE_GRACE_NANOS_INFINITE;
   private long keepAliveTimeInNanos = DEFAULT_SERVER_KEEPALIVE_TIME_NANOS;
   private long keepAliveTimeoutInNanos = DEFAULT_SERVER_KEEPALIVE_TIMEOUT_NANOS;
-  private TransportTracer transportTracer;
+  private TransportTracer transportTracer = new TransportTracer();
 
   private class ServerTransportListenerImpl implements ServerTransportListener {
 
@@ -155,12 +155,10 @@ public class NettyServerHandlerTest extends NettyHandlerTestBase<NettyServerHand
     }
   }
 
-  @Override
-  protected void manualSetUp() throws Exception {
-    assertNull("manualSetUp should not run more than once", handler());
-
+  @Before
+  public void setUp() {
     MockitoAnnotations.initMocks(this);
-    transportTracer = new TransportTracer();
+
     when(streamTracerFactory.newServerStreamTracer(anyString(), any(Metadata.class)))
         .thenReturn(streamTracer);
 
@@ -178,7 +176,12 @@ public class NettyServerHandlerTest extends NettyHandlerTestBase<NettyServerHand
             }
           })
       .when(streamListener)
-      .messagesAvailable(Matchers.<StreamListener.MessageProducer>any());
+      .messagesAvailable(any(StreamListener.MessageProducer.class));
+  }
+
+  @Override
+  protected void manualSetUp() throws Exception {
+    assertNull("manualSetUp should not run more than once", handler());
 
     initChannel(new GrpcHttp2ServerHeadersDecoder(GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE));
 
@@ -195,6 +198,19 @@ public class NettyServerHandlerTest extends NettyHandlerTestBase<NettyServerHand
     channelRead(serializedSettings);
   }
 
+  @Test
+  public void transportReadyDelayedUntilConnectionPreface() throws Exception {
+    initChannel(new GrpcHttp2ServerHeadersDecoder(GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE));
+
+    handler().handleProtocolNegotiationCompleted(Attributes.EMPTY);
+    verify(transportListener, never()).transportReady(any(Attributes.class));
+
+    // Simulate receipt of the connection preface
+    channelRead(Http2CodecUtil.connectionPrefaceBuf());
+    channelRead(serializeSettings(new Http2Settings()));
+    verify(transportListener).transportReady(any(Attributes.class));
+  }
+
   @Test
   public void sendFrameShouldSucceed() throws Exception {
     manualSetUp();
-- 
cgit v1.2.3