Reverse order of closing and completing the promise

May fix #7464.
bazelbuild · Feb 22, 2019 · 8e5a30a · 8e5a30a
1 parent 800e7f2
commit 8e5a30a
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 48 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/remote/blobstore/http/AbstractHttpHandler.java b/src/main/java/com/google/devtools/build/lib/remote/blobstore/http/AbstractHttpHandler.java
@@ -99,8 +99,8 @@ protected String constructHost(URI uri) {
 
   @Override
   public void exceptionCaught(ChannelHandlerContext ctx, Throwable t) {
-    failAndResetUserPromise(t);
     ctx.fireExceptionCaught(t);
+    failAndResetUserPromise(t);
   }
 
   @SuppressWarnings("FutureReturnValueIgnored")
@@ -119,25 +119,22 @@ public void connect(
     ctx.connect(remoteAddress, localAddress, promise);
   }
 
-  @SuppressWarnings("FutureReturnValueIgnored")
   @Override
   public void disconnect(ChannelHandlerContext ctx, ChannelPromise promise) {
-    failAndResetUserPromise(new ClosedChannelException());
-    ctx.disconnect(promise);
+    ctx.disconnect(promise)
+        .addListener((f) -> failAndResetUserPromise(new ClosedChannelException()));
   }
 
-  @SuppressWarnings("FutureReturnValueIgnored")
   @Override
   public void close(ChannelHandlerContext ctx, ChannelPromise promise) {
-    failAndResetUserPromise(new ClosedChannelException());
-    ctx.close(promise);
+    ctx.close(promise)
+        .addListener((f) -> failAndResetUserPromise(new ClosedChannelException()));
   }
 
-  @SuppressWarnings("FutureReturnValueIgnored")
   @Override
   public void deregister(ChannelHandlerContext ctx, ChannelPromise promise) {
-    failAndResetUserPromise(new ClosedChannelException());
-    ctx.deregister(promise);
+    ctx.deregister(promise)
+        .addListener((f) -> failAndResetUserPromise(new ClosedChannelException()));
   }
 
   @SuppressWarnings("FutureReturnValueIgnored")
@@ -154,8 +151,8 @@ public void flush(ChannelHandlerContext ctx) {
 
   @Override
   public void channelInactive(ChannelHandlerContext ctx) {
-    failAndResetUserPromise(new ClosedChannelException());
     ctx.fireChannelInactive();
+    failAndResetUserPromise(new ClosedChannelException());
   }
 
   @Override
@@ -165,7 +162,7 @@ public void handlerRemoved(ChannelHandlerContext ctx) {
 
   @Override
   public void channelUnregistered(ChannelHandlerContext ctx) {
-    failAndResetUserPromise(new ClosedChannelException());
     ctx.fireChannelUnregistered();
+    failAndResetUserPromise(new ClosedChannelException());
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/blobstore/http/HttpDownloadHandler.java b/src/main/java/com/google/devtools/build/lib/remote/blobstore/http/HttpDownloadHandler.java
@@ -17,6 +17,7 @@
 
 import com.google.auth.Credentials;
 import io.netty.buffer.ByteBuf;
+import io.netty.channel.ChannelFuture;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelPromise;
 import io.netty.handler.codec.http.DefaultFullHttpRequest;
@@ -164,41 +165,32 @@ private HttpRequest buildRequest(String path, String host) {
   }
 
   private void succeedAndReset(ChannelHandlerContext ctx) {
-    try {
+    if (keepAlive) {
+      resetState();
       succeedAndResetUserPromise();
-    } finally {
-      reset(ctx);
+    } else {
+      ctx.close().addListener((f) -> succeedAndResetUserPromise());
     }
   }
 
   @SuppressWarnings("FutureReturnValueIgnored")
   private void failAndClose(Throwable t, ChannelHandlerContext ctx) {
-    try {
-      failAndResetUserPromise(t);
-    } finally {
-      ctx.close();
-    }
+    ctx.close().addListener((f) -> failAndResetUserPromise(t));
   }
 
   private void failAndReset(Throwable t, ChannelHandlerContext ctx) {
-    try {
+    if (keepAlive) {
+      resetState();
       failAndResetUserPromise(t);
-    } finally {
-      reset(ctx);
+    } else {
+      ctx.close().addListener((f) -> failAndResetUserPromise(t));
     }
   }
 
-  @SuppressWarnings("FutureReturnValueIgnored")
-  private void reset(ChannelHandlerContext ctx) {
-    try {
-      if (!keepAlive) {
-        ctx.close();
-      }
-    } finally {
-      out = null;
-      keepAlive = HttpVersion.HTTP_1_1.isKeepAliveDefault();
-      downloadSucceeded = false;
-      response = null;
-    }
+  private void resetState() {
+    out = null;
+    keepAlive = HttpVersion.HTTP_1_1.isKeepAliveDefault();
+    downloadSucceeded = false;
+    response = null;
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/remote/blobstore/http/HttpUploadHandler.java b/src/main/java/com/google/devtools/build/lib/remote/blobstore/http/HttpUploadHandler.java
@@ -53,20 +53,31 @@ protected void channelRead0(ChannelHandlerContext ctx, FullHttpResponse response
           && !response.status().equals(HttpResponseStatus.CREATED)
           && !response.status().equals(HttpResponseStatus.NO_CONTENT)) {
         // Supporting more than OK status to be compatible with nginx webdav.
-        String errorMsg = response.status().toString();
+        String errorMsgBuilder = response.status().toString();
         if (response.content().readableBytes() > 0) {
           byte[] data = new byte[response.content().readableBytes()];
           response.content().readBytes(data);
-          errorMsg += "\n" + new String(data, HttpUtil.getCharset(response));
+          errorMsgBuilder += "\n" + new String(data, HttpUtil.getCharset(response));
+        }
+        String errorMsg = errorMsgBuilder;
+        if (!HttpUtil.isKeepAlive(response)) {
+          ctx.close()
+              .addListener(
+                  (f) -> failAndResetUserPromise(new HttpException(response, errorMsg, null)));
+        } else {
+          failAndResetUserPromise(new HttpException(response, errorMsg, null));
         }
-        failAndResetUserPromise(new HttpException(response, errorMsg, null));
       } else {
-        succeedAndResetUserPromise();
-      }
-    } finally {
-      if (!HttpUtil.isKeepAlive(response)) {
-        ctx.close();
+        if (!HttpUtil.isKeepAlive(response)) {
+          ctx.close()
+              .addListener((f) -> succeedAndResetUserPromise());
+        } else {
+          succeedAndResetUserPromise();
+        }
       }
+    } catch (RuntimeException e) {
+      ctx.close();
+      throw e;
     }
   }
 
@@ -122,10 +133,6 @@ private HttpChunkedInput buildBody(UploadCommand msg) {
 
   @SuppressWarnings("FutureReturnValueIgnored")
   private void failAndClose(Throwable t, ChannelHandlerContext ctx) {
-    try {
-      failAndResetUserPromise(t);
-    } finally {
-      ctx.close();
-    }
+    ctx.close().addListener((f) -> failAndResetUserPromise(t));
   }
 }