[ 
https://issues.apache.org/jira/browse/TINKERPOP-2374?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17119277#comment-17119277
 ] 

ASF GitHub Bot commented on TINKERPOP-2374:
-------------------------------------------

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


> SaslAndHttpBasicAuthenticationHandler can't extract authorization
> -----------------------------------------------------------------
>
>                 Key: TINKERPOP-2374
>                 URL: https://issues.apache.org/jira/browse/TINKERPOP-2374
>             Project: TinkerPop
>          Issue Type: Bug
>            Reporter: Jermy Li
>            Priority: Major
>
> When we use the following configuration and keep http connection alive, some 
> requests will fail to get authorization information during consecutive 
> requests.
> {code:yaml}
> channelizer: org.apache.tinkerpop.gremlin.server.channel.WsAndHttpChannelizer
> authentication: {
>   authenticationHandler: 
> org.apache.tinkerpop.gremlin.server.handler.SaslAndHttpBasicAuthenticationHandler,
> }
> {code}
>  
> We expect the sequence in the pipeline to be:
> {code:java}
> (http-response-encoder = io.netty.handler.codec.http.HttpResponseEncoder), 
> (authenticator = 
> org.apache.tinkerpop.gremlin.server.handler.SaslAndHttpBasicAuthenticationHandler),
>  
> (http-authentication = 
> org.apache.tinkerpop.gremlin.server.handler.HttpBasicAuthenticationHandler),
> (request-handler = 
> org.apache.tinkerpop.gremlin.server.handler.HttpGremlinEndpointHandler), 
> {code}
> authenticator -> {color:#ff0000}http-authentication{color} -> request-handler
> But sometimes its order becomes the following, so that user information 
> cannot be obtained:
> {code:java}
> (http-response-encoder = io.netty.handler.codec.http.HttpResponseEncoder), 
> (authenticator = 
> org.apache.tinkerpop.gremlin.server.handler.SaslAndHttpBasicAuthenticationHandler),
>  
> (request-handler = 
> org.apache.tinkerpop.gremlin.server.handler.HttpGremlinEndpointHandler), 
> (http-authentication = 
> org.apache.tinkerpop.gremlin.server.handler.HttpBasicAuthenticationHandler),
> {code}
> authenticator -> request-handler -> {color:#ff0000}http-authentication{color}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Reply via email to