remote/http: properly support read timeout

Also, remove unused SO_TIMEOUT. Fixes #4890

cc @benjaminp

Closes #4895.

PiperOrigin-RevId: 190051030

2 files changed, 51 insertions, 4 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/remote/blobstore/http/HttpBlobStore.java b/src/main/java/com/google/devtools/build/lib/remote/blobstore/http/HttpBlobStore.java
index bb10623913..012931bade 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/blobstore/http/HttpBlobStore.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/blobstore/http/HttpBlobStore.java
@@ -38,6 +38,7 @@ import io.netty.handler.ssl.SslContextBuilder;
 import io.netty.handler.ssl.SslHandler;
 import io.netty.handler.ssl.SslProvider;
 import io.netty.handler.stream.ChunkedWriteHandler;
+import io.netty.handler.timeout.ReadTimeoutHandler;
 import io.netty.util.internal.PlatformDependent;
 import java.io.ByteArrayInputStream;
 import java.io.FileInputStream;
@@ -127,7 +128,6 @@ public final class HttpBlobStore implements SimpleBlobStore {
         new Bootstrap()
             .channel(NioSocketChannel.class)
             .option(ChannelOption.CONNECT_TIMEOUT_MILLIS, timeoutMillis)
-            .option(ChannelOption.SO_TIMEOUT, timeoutMillis)
             .group(eventLoop)
             .remoteAddress(uri.getHost(), uri.getPort());
     downloadChannels =
@@ -135,14 +135,20 @@ public final class HttpBlobStore implements SimpleBlobStore {
             clientBootstrap,
             new ChannelPoolHandler() {
               @Override
-              public void channelReleased(Channel ch) {}
+              public void channelReleased(Channel ch) {
+                ch.pipeline().remove("read-timeout-handler");
+              }
 
               @Override
-              public void channelAcquired(Channel ch) {}
+              public void channelAcquired(Channel ch) {
+                ch.pipeline()
+                    .addFirst("read-timeout-handler", new ReadTimeoutHandler(timeoutMillis));
+              }
 
               @Override
               public void channelCreated(Channel ch) {
                 ChannelPipeline p = ch.pipeline();
+                p.addFirst("read-timeout-handler", new ReadTimeoutHandler(timeoutMillis));
                 if (sslCtx != null) {
                   SSLEngine engine = sslCtx.newEngine(ch.alloc());
                   engine.setUseClientMode(true);
@@ -213,7 +219,7 @@ public final class HttpBlobStore implements SimpleBlobStore {
           }
         };
     DownloadCommand download = new DownloadCommand(uri, casDownload, key, wrappedOut);
-    ;
+
     Channel ch = null;
     try {
       ch = acquireDownloadChannel();
diff --git a/src/test/java/com/google/devtools/build/lib/remote/blobstore/http/HttpBlobStoreTest.java b/src/test/java/com/google/devtools/build/lib/remote/blobstore/http/HttpBlobStoreTest.java
index 7854cb53e7..6850f9e17d 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/blobstore/http/HttpBlobStoreTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/blobstore/http/HttpBlobStoreTest.java
@@ -32,6 +32,7 @@ import io.netty.buffer.ByteBuf;
 import io.netty.channel.ChannelFutureListener;
 import io.netty.channel.ChannelHandler;
 import io.netty.channel.ChannelHandler.Sharable;
+import io.netty.channel.ChannelHandlerAdapter;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelInitializer;
 import io.netty.channel.EventLoopGroup;
@@ -49,8 +50,10 @@ import io.netty.handler.codec.http.HttpResponseStatus;
 import io.netty.handler.codec.http.HttpServerCodec;
 import io.netty.handler.codec.http.HttpUtil;
 import io.netty.handler.codec.http.HttpVersion;
+import io.netty.handler.timeout.ReadTimeoutException;
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
+import java.net.ConnectException;
 import java.net.URI;
 import java.util.HashMap;
 import java.util.List;
@@ -82,6 +85,44 @@ public class HttpBlobStoreTest {
     return ((ServerSocketChannel) sb.bind("localhost", 0).sync().channel());
   }
 
+  @Test(expected = ConnectException.class, timeout = 30000)
+  public void timeoutShouldWork_connect() throws Exception {
+    ServerSocketChannel server = startServer(new ChannelHandlerAdapter() {});
+    int serverPort = server.localAddress().getPort();
+    closeServerChannel(server);
+
+    Credentials credentials = newCredentials();
+    HttpBlobStore blobStore =
+        new HttpBlobStore(new URI("http://localhost:" + serverPort), 5, credentials);
+    blobStore.get("key", new ByteArrayOutputStream());
+    fail("Exception expected");
+  }
+
+  @Test(expected = ReadTimeoutException.class, timeout = 30000)
+  public void timeoutShouldWork_read() throws Exception {
+    ServerSocketChannel server = null;
+    try {
+      server =
+          startServer(
+              new SimpleChannelInboundHandler<FullHttpRequest>() {
+                @Override
+                protected void channelRead0(
+                    ChannelHandlerContext channelHandlerContext, FullHttpRequest fullHttpRequest) {
+                  // Don't respond and force a client timeout.
+                }
+              });
+      int serverPort = server.localAddress().getPort();
+
+      Credentials credentials = newCredentials();
+      HttpBlobStore blobStore =
+          new HttpBlobStore(new URI("http://localhost:" + serverPort), 5, credentials);
+      blobStore.get("key", new ByteArrayOutputStream());
+      fail("Exception expected");
+    } finally {
+      closeServerChannel(server);
+    }
+  }
+
   @Test
   public void expiredAuthTokensShouldBeRetried_get() throws Exception {
     expiredAuthTokensShouldBeRetried_get(ErrorType.UNAUTHORIZED);