Fix races in the HTTP upload and download handlers

The user promise has a callback that returns the connection to the pool.
If the server returns a 'connection: close' HTTP header, then this can
currently happen before the connection is closed, in which case the client
attempts to reuse the connection, which - of course - fails.

This changes the ordering to close the connection *before* completing the
user promise.

This is at least a partial fix for the linked issue. It is unclear if this
is the root cause for all the reported failure modes.

Progress on bazelbuild#10159.

Change-Id: I2897e55c6edda592a6fb5755ddcccd1a89cde528

src/main/java/com/google/devtools/build/lib/remote/http/HttpDownloadHandler.java

@@ -170,28 +170,21 @@ private HttpRequest buildRequest(String path, String host) {
   }
 
   private void succeedAndReset(ChannelHandlerContext ctx) {
-    try {
-      succeedAndResetUserPromise();
-    } finally {
-      reset(ctx);
-    }
+    // All resets must happen *before* completing the user promise. Otherwise there is a race
+    // condition, where this handler can be reused even though it is closed.
+    reset(ctx);
+    succeedAndResetUserPromise();
   }
 
   @SuppressWarnings("FutureReturnValueIgnored")
   private void failAndClose(Throwable t, ChannelHandlerContext ctx) {
-    try {
-      failAndResetUserPromise(t);
-    } finally {
-      ctx.close();
-    }
+    ctx.close();
+    failAndResetUserPromise(t);
   }
 
   private void failAndReset(Throwable t, ChannelHandlerContext ctx) {
-    try {
-      failAndResetUserPromise(t);
-    } finally {
-      reset(ctx);
-    }
+    reset(ctx);
+    failAndResetUserPromise(t);
   }
 
   @SuppressWarnings("FutureReturnValueIgnored")

src/main/java/com/google/devtools/build/lib/remote/http/HttpUploadHandler.java

@@ -56,27 +56,27 @@ protected void channelRead0(ChannelHandlerContext ctx, FullHttpResponse response
       failAndClose(new IOException("Failed to parse the HTTP response."), ctx);
       return;
     }
-    try {
-      checkState(userPromise != null, "response before request");
-      if (!response.status().equals(HttpResponseStatus.OK)
-          && !response.status().equals(HttpResponseStatus.ACCEPTED)
-          && !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();
-        if (response.content().readableBytes() > 0) {
-          byte[] data = new byte[response.content().readableBytes()];
-          response.content().readBytes(data);
-          errorMsg += "\n" + new String(data, HttpUtil.getCharset(response));
-        }
-        failAndResetUserPromise(new HttpException(response, errorMsg, null));
-      } else {
-        succeedAndResetUserPromise();
-      }
-    } finally {
-      if (!HttpUtil.isKeepAlive(response)) {
-        ctx.close();
+
+    // Connection reset must happen *before* completing the user promise. Otherwise there is a race
+    // condition, where this handler can be reused even though it is closed.
+    if (!HttpUtil.isKeepAlive(response)) {
+      ctx.close();
+    }
+    checkState(userPromise != null, "response before request");
+    if (!response.status().equals(HttpResponseStatus.OK)
+        && !response.status().equals(HttpResponseStatus.ACCEPTED)
+        && !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();
+      if (response.content().readableBytes() > 0) {
+        byte[] data = new byte[response.content().readableBytes()];
+        response.content().readBytes(data);
+        errorMsg += "\n" + new String(data, HttpUtil.getCharset(response));
       }
+      failAndResetUserPromise(new HttpException(response, errorMsg, null));
+    } else {
+      succeedAndResetUserPromise();
     }
   }
 
@@ -144,10 +144,9 @@ private HttpChunkedInput buildBody(UploadCommand msg) {
 
   @SuppressWarnings("FutureReturnValueIgnored")
   private void failAndClose(Throwable t, ChannelHandlerContext ctx) {
-    try {
-      failAndResetUserPromise(t);
-    } finally {
-      ctx.close();
-    }
+    // All resets must happen *before* completing the user promise. Otherwise there is a race
+    // condition, where this handler can be reused even though it is closed.
+    ctx.close();
+    failAndResetUserPromise(t);
   }
 }