divijvaidya commented on code in PR #1833:
URL: https://github.com/apache/tinkerpop/pull/1833#discussion_r1034726914
##########
gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/WebSocketClientBehaviorIntegrateTest.java:
##########
@@ -271,4 +276,28 @@ public void
shouldNotCreateReplacementConnectionWhenClientClosesConnection() thr
.filter(str -> str.contains("Considering new
connection on"))
.count());
}
-}
\ No newline at end of file
+
+ /**
+ * (TINKERPOP-2814) Tests to make sure that the SSL handshake is now
capped by connectionSetupTimeoutMillis and not
+ * the default Netty SSL handshake timeout of 10,000ms.
+ */
+ @Test
+ public void
shouldAttemptHandshakeForLongerThanDefaultNettySslHandshakeTimeout() {
+ final Cluster cluster =
Cluster.build("localhost").port(SimpleSocketServer.PORT)
+ .minConnectionPoolSize(1)
+ .maxConnectionPoolSize(1)
+ .connectionSetupTimeoutMillis(20000) // needs to be larger
than 10,000ms.
+ .enableSsl(true)
+ .create();
+
+ final Client.ClusteredClient client = cluster.connect();
+ final long start = System.currentTimeMillis();
+
+ try {
+ client.submit("1");
+ } finally {
+ // Test against 15,000ms which should give a big enough buffer to
avoid timing issues.
Review Comment:
nit
not important but FYI, in some geographies (such as Germany) a comma in
numbers is read as decimal and folks in Germany will probably read this as
15ms! Not using comma at all keeps things independent of such geographic
irregularities.
##########
gremlin-driver/src/main/java/org/apache/tinkerpop/gremlin/driver/Channelizer.java:
##########
@@ -123,7 +124,9 @@ protected void initChannel(final SocketChannel
socketChannel) throws Exception {
}
if (sslCtx.isPresent()) {
-
pipeline.addLast(sslCtx.get().newHandler(socketChannel.alloc(),
connection.getUri().getHost(), connection.getUri().getPort()));
+ SslHandler sslHandler =
sslCtx.get().newHandler(socketChannel.alloc(), connection.getUri().getHost(),
connection.getUri().getPort());
+ sslHandler.setHandshakeTimeoutMillis(0);
Review Comment:
please add a comment in the code explaining the motivation of setting this
to 0. It would be helpful for future contributors.
##########
gremlin-driver/src/test/java/org/apache/tinkerpop/gremlin/driver/WebSocketClientBehaviorIntegrateTest.java:
##########
@@ -271,4 +276,28 @@ public void
shouldNotCreateReplacementConnectionWhenClientClosesConnection() thr
.filter(str -> str.contains("Considering new
connection on"))
.count());
}
-}
\ No newline at end of file
+
+ /**
+ * (TINKERPOP-2814) Tests to make sure that the SSL handshake is now
capped by connectionSetupTimeoutMillis and not
+ * the default Netty SSL handshake timeout of 10,000ms.
+ */
+ @Test
+ public void
shouldAttemptHandshakeForLongerThanDefaultNettySslHandshakeTimeout() {
+ final Cluster cluster =
Cluster.build("localhost").port(SimpleSocketServer.PORT)
+ .minConnectionPoolSize(1)
+ .maxConnectionPoolSize(1)
+ .connectionSetupTimeoutMillis(20000) // needs to be larger
than 10,000ms.
+ .enableSsl(true)
+ .create();
+
+ final Client.ClusteredClient client = cluster.connect();
+ final long start = System.currentTimeMillis();
+
+ try {
+ client.submit("1");
+ } finally {
+ // Test against 15,000ms which should give a big enough buffer to
avoid timing issues.
+ assertTrue(System.currentTimeMillis() - start > 15000);
Review Comment:
If I understand this correctly, we expect the connection setup to fail and
we verify that the failure is after the threshold of ssl handshake timeout,
hence proving that the timeout is bound by the overall connection setup limit
and not by ssl. Is that right?
Could we also add an assertion that we expect the connection setup to fail?
I know that it is not what this test is checking but it will help guard the
purpose of this test is future cases. Also let's validate that the correct
error message is being printed and we haven't regressed in the aspect of
providing the detailed/correct error log.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
