divijvaidya commented on a change in pull request #1289: URL: https://github.com/apache/tinkerpop/pull/1289#discussion_r432228859
########## File path: gremlin-server/src/main/java/org/apache/tinkerpop/gremlin/server/handler/SaslAndHttpBasicAuthenticationHandler.java ########## @@ -47,9 +47,11 @@ public SaslAndHttpBasicAuthenticationHandler(final Authenticator authenticator, @Override public void channelRead(final ChannelHandlerContext ctx, final Object obj) throws Exception { if (obj instanceof HttpMessage && !WebSocketHandlerUtil.isWebSocket((HttpMessage)obj)) { - if (null == ctx.pipeline().get(HTTP_AUTH)) { - ctx.pipeline().addAfter(PIPELINE_AUTHENTICATOR, HTTP_AUTH, new HttpBasicAuthenticationHandler(authenticator, this.authenticationSettings)); + ChannelPipeline pipeline = ctx.pipeline(); + if (null != pipeline.get(HTTP_AUTH)) { + pipeline.remove(HTTP_AUTH); } Review comment: This code change doesn't really solve the root cause and doesn't explain why are having random pipeline behaviour. Here's my theory: When keep alive is turned on, multiple HTTP requests use the same pipeline. This causes a race condition where multiple requests are trying to modify the pipeline dynamically in channelRead method of `WsAndHttpChannelizerHandler`. While one message is dynamically modifying the pipeline, let's say executing line 68 (removing the PIPELINE_AUTHENTICATOR), another message might come in and execute line 65 and erroneously jumped to line 71. This causes the unpredictable behaviour you are observing. The correct fix would be to ensure that the above dynamic modification of pipeline is only done once. IMO we should not push this workaround without fixing the underlying root cause. WDYT @spmallette ? ---------------------------------------------------------------- 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: us...@infra.apache.org