kenhuuu commented on code in PR #1833:
URL: https://github.com/apache/tinkerpop/pull/1833#discussion_r1037713064
##########
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:
I recently noticed an exception that is being thrown from the SslHandler
when setting this value to 0.
`[ERROR] org.apache.tinkerpop.gremlin.driver.Handler$GremlinResponseHandler
- Could not process the response
io.netty.handler.ssl.SslClosedEngineException: SSLEngine closed already
at io.netty.handler.ssl.SslHandler.wrap(SslHandler.java:859)
at io.netty.handler.ssl.SslHandler.wrapAndFlush(SslHandler.java:798)
at io.netty.handler.ssl.SslHandler.flush(SslHandler.java:779)
at io.netty.handler.ssl.SslHandler.flush(SslHandler.java:1971)
at
io.netty.handler.ssl.SslHandler.closeOutboundAndChannel(SslHandler.java:1940)
at io.netty.handler.ssl.SslHandler.close(SslHandler.java:730)
at
io.netty.channel.AbstractChannelHandlerContext.invokeClose(AbstractChannelHandlerContext.java:622)
at
io.netty.channel.AbstractChannelHandlerContext.close(AbstractChannelHandlerContext.java:606)
at
io.netty.channel.CombinedChannelDuplexHandler$DelegatingChannelHandlerContext.close(CombinedChannelDuplexHandler.java:505)
at
io.netty.channel.ChannelOutboundHandlerAdapter.close(ChannelOutboundHandlerAdapter.java:77)
at
io.netty.channel.CombinedChannelDuplexHandler.close(CombinedChannelDuplexHandler.java:316)
at
io.netty.channel.AbstractChannelHandlerContext.invokeClose(AbstractChannelHandlerContext.java:622)
at
io.netty.channel.AbstractChannelHandlerContext.close(AbstractChannelHandlerContext.java:606)
at
io.netty.channel.AbstractChannelHandlerContext.close(AbstractChannelHandlerContext.java:472)
at
io.netty.handler.codec.http.websocketx.WebSocketClientProtocolHandshakeHandler$2.run(WebSocketClientProtocolHandshakeHandler.java:122)
at io.netty.util.concurrent.PromiseTask.runTask(PromiseTask.java:98)
at
io.netty.util.concurrent.ScheduledFutureTask.run(ScheduledFutureTask.java:170)
at
io.netty.util.concurrent.AbstractEventExecutor.runTask(AbstractEventExecutor.java:174)
at
io.netty.util.concurrent.AbstractEventExecutor.safeExecute(AbstractEventExecutor.java:167)
at
io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:470)
at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:503)
at
io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:995)
at
io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
at java.base/java.lang.Thread.run(Thread.java:829)`
I've tried several workarounds to get rid of this issue with closing the
SslHandler. These workarounds include subclassing SslHandler to modify how
close works if the handshake times out, removing the SslHandler from the
pipeline and swallowing the exception if the handshake failed. However, all of
these workarounds have drawbacks so instead I propose that we go back to your
initial recommendation which is to make the SSL handshake timeout a percentage
of `connectionSetupTimeoutMillis`. I feel like 0.9*connectionSetupTimeoutMillis
would still enable configurability for this value while preventing any timing
issues with the WebSocket handshake timeout.
What are you thoughts on this @divijvaidya ? Thanks.
--
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]
