[GitHub] [geode] dschneider-pivotal commented on a change in pull request #5665: GEODE-8651: MsgReader's readHeader and readMessage should be synchron…

[GitBox <gi...@apache.org>]

dschneider-pivotal commented on a change in pull request #5665:
URL: https://github.com/apache/geode/pull/5665#discussion_r512899430



##########
File path: geode-core/src/test/java/org/apache/geode/internal/tcp/ConnectionTest.java
##########
@@ -113,4 +114,35 @@ public void connectTimeoutIsShortWhenAlerting() throws UnknownHostException {
       assertThat(connection.getP2PConnectTimeout(distributionConfig)).isEqualTo(100);
     });
   }
+
+  @Test
+  public void notifyHandshakeWaiterShouldPositionByteBufferOnlyOnce() throws Exception {
+    ConnectionTable connectionTable = mock(ConnectionTable.class);
+    Distribution distribution = mock(Distribution.class);
+    DistributionManager distributionManager = mock(DistributionManager.class);
+    DMStats dmStats = mock(DMStats.class);
+    CancelCriterion stopper = mock(CancelCriterion.class);
+    SocketCloser socketCloser = mock(SocketCloser.class);
+    TCPConduit tcpConduit = mock(TCPConduit.class);
+
+    when(connectionTable.getBufferPool()).thenReturn(new BufferPool(dmStats));
+    when(connectionTable.getConduit()).thenReturn(tcpConduit);
+    when(connectionTable.getDM()).thenReturn(distributionManager);
+    when(connectionTable.getSocketCloser()).thenReturn(socketCloser);
+    when(distributionManager.getDistribution()).thenReturn(distribution);
+    when(stopper.cancelInProgress()).thenReturn(null);
+    when(tcpConduit.getCancelCriterion()).thenReturn(stopper);
+    when(tcpConduit.getDM()).thenReturn(distributionManager);
+    when(tcpConduit.getSocketId()).thenReturn(new InetSocketAddress(getLocalHost(), 10337));
+    when(tcpConduit.getStats()).thenReturn(dmStats);
+
+    SocketChannel channel = SocketChannel.open();
+
+    Connection connection = new Connection(connectionTable, channel.socket());
+    connection = spy(connection);
+    connection.setSharedUnorderedForTest();
+    connection.run();
+
+    verify(connection, times(1)).getConduit();

Review comment:
       I don't like that we verify that getConduit is called 1 time. I understand after looking at the code but it seems rather indirect logically and brittle. I would prefer that you extract the block of code in notifyHandshakeWaiter starting at line 810 into a method named "clearSSLInputBuffer". Then this test can assert that clearSSLInputBuffer is only called 1 time. This would be much clearer and less brittle and would not increase the diffs much




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org