[
https://issues.apache.org/jira/browse/TINKERPOP-2814?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17640693#comment-17640693
]
ASF GitHub Bot commented on TINKERPOP-2814:
-------------------------------------------
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.
> Add a SSL handshake timeout configuration to the driver
> -------------------------------------------------------
>
> Key: TINKERPOP-2814
> URL: https://issues.apache.org/jira/browse/TINKERPOP-2814
> Project: TinkerPop
> Issue Type: Improvement
> Components: driver
> Affects Versions: 3.5.4
> Reporter: Stephen Mallette
> Priority: Blocker
>
> The java driver currently relies on the default 10 second SSL handshake
> timeout defined by Netty. Add a configuration to the driver to allow users to
> change that setting.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
