Repository: netty/netty
Issue: ProxyHandler stalls the proxy handshake when auto-read=false
Labels: defect

We're (Finagle) using the N4 tooling around proxies and just discovered a bug in [`ProxyHandler`](https://github.com/netty/netty/blob/4.1/handler-proxy/src/main/java/io/netty/handler/proxy/ProxyHandler.java). When auto-read is disabled (our default) a connection established with the proxy server goes into a stall state waiting for a first response and never receives that (fires a handshake timeout). This is a common problem and might be reproduced with both HTTP and SOCKS5.

While debugging this we noticed that a response was actually sent by a server, but was never read from the pipeline by a `ProxyHandler` since [it doesn't issue a read-request explicitly](https://github.com/netty/netty/blob/4.1/handler-proxy/src/main/java/io/netty/handler/proxy/ProxyHandler.java#L194-L211) and relies on auto-read (or previous handlers to issue the read).

The solution to this bug is pretty straightforward - we have to issue a read request (once for HTTP and twice for SOCKS5) if the auto-read is disabled.

We're working on the PR (with the fix) right now.


Issue comments:
Adding some new details. Turns out we only need to issue one read to trigger the handshake (no matter how many messages it takes to complete for HTTP and SOCKS5) given that [`ProxyHandler.readComplete()`](https://github.com/netty/netty/blob/4.1/handler-proxy/src/main/java/io/netty/handler/proxy/ProxyHandler.java#L387-L389) will keep reading for us.

Fixed by https://github.com/netty/netty/pull/5939


Pull request review comments:
ThreadLocalRandom.current().nextBoolean()

Thanks @normanmaurer! Just updated the branch.


Patch that resolved the issue:
diff --git a/handler-proxy/src/main/java/io/netty/handler/proxy/ProxyHandler.java b/handler-proxy/src/main/java/io/netty/handler/proxy/ProxyHandler.java
index 8d5b80b0aad..756de090a0a 100644
--- a/handler-proxy/src/main/java/io/netty/handler/proxy/ProxyHandler.java
+++ b/handler-proxy/src/main/java/io/netty/handler/proxy/ProxyHandler.java
@@ -208,6 +208,8 @@ public void run() {
         if (initialMessage != null) {
             sendToProxyServer(initialMessage);
         }
+
+        readIfNeeded(ctx);
     }
 
     /**
@@ -384,9 +386,7 @@ public final void channelReadComplete(ChannelHandlerContext ctx) throws Exceptio
         if (suppressChannelReadComplete) {
             suppressChannelReadComplete = false;
 
-            if (!ctx.channel().config().isAutoRead()) {
-                ctx.read();
-            }
+            readIfNeeded(ctx);
         } else {
             ctx.fireChannelReadComplete();
         }
@@ -412,6 +412,12 @@ public final void flush(ChannelHandlerContext ctx) throws Exception {
         }
     }
 
+    private static void readIfNeeded(ChannelHandlerContext ctx) {
+        if (!ctx.channel().config().isAutoRead()) {
+            ctx.read();
+        }
+    }
+
     private void writePendingWrites() {
         if (pendingWrites != null) {
             pendingWrites.removeAndWriteAll();


Test patch:
diff --git a/handler-proxy/src/test/java/io/netty/handler/proxy/ProxyHandlerTest.java b/handler-proxy/src/test/java/io/netty/handler/proxy/ProxyHandlerTest.java
index 1c5cb5ec06a..697b57bfe41 100644
--- a/handler-proxy/src/test/java/io/netty/handler/proxy/ProxyHandlerTest.java
+++ b/handler-proxy/src/test/java/io/netty/handler/proxy/ProxyHandlerTest.java
@@ -26,6 +26,7 @@
 import io.netty.channel.ChannelHandler;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelInitializer;
+import io.netty.channel.ChannelOption;
 import io.netty.channel.ChannelPipeline;
 import io.netty.channel.EventLoopGroup;
 import io.netty.channel.SimpleChannelInboundHandler;
@@ -61,6 +62,7 @@
 import java.util.Queue;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.LinkedBlockingQueue;
+import java.util.concurrent.ThreadLocalRandom;
 import java.util.concurrent.TimeUnit;
 
 import static org.hamcrest.CoreMatchers.*;
@@ -363,9 +365,16 @@ private static final class SuccessTestHandler extends SimpleChannelInboundHandle
         final Queue<Throwable> exceptions = new LinkedBlockingQueue<Throwable>();
         volatile int eventCount;
 
+        private static void readIfNeeded(ChannelHandlerContext ctx) {
+            if (!ctx.channel().config().isAutoRead()) {
+                ctx.read();
+            }
+        }
+
         @Override
         public void channelActive(ChannelHandlerContext ctx) throws Exception {
             ctx.writeAndFlush(Unpooled.copiedBuffer("A\n", CharsetUtil.US_ASCII));
+            readIfNeeded(ctx);
         }
 
         @Override
@@ -378,6 +387,7 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc
                     // ProxyHandlers in the pipeline.  Therefore, we send the 'B' message only on the first event.
                     ctx.writeAndFlush(Unpooled.copiedBuffer("B\n", CharsetUtil.US_ASCII));
                 }
+                readIfNeeded(ctx);
             }
         }
 
@@ -388,6 +398,7 @@ protected void channelRead0(ChannelHandlerContext ctx, Object msg) throws Except
             if ("2".equals(str)) {
                 ctx.writeAndFlush(Unpooled.copiedBuffer("C\n", CharsetUtil.US_ASCII));
             }
+            readIfNeeded(ctx);
         }
 
         @Override
@@ -523,6 +534,7 @@ protected void test() throws Exception {
             Bootstrap b = new Bootstrap();
             b.group(group);
             b.channel(NioSocketChannel.class);
+            b.option(ChannelOption.AUTO_READ, ThreadLocalRandom.current().nextBoolean());
             b.resolver(NoopAddressResolverGroup.INSTANCE);
             b.handler(new ChannelInitializer<SocketChannel>() {
                 @Override