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 #10159.

Change-Id: I2897e55c6edda592a6fb5755ddcccd1a89cde528

Closes #12055.

Change-Id: I2897e55c6edda592a6fb5755ddcccd1a89cde528
PiperOrigin-RevId: 330496714

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

@@ -59,12 +59,6 @@ protected void failAndResetUserPromise(Throwable t) {
     userPromise = null;
   }
 
-  @SuppressWarnings("FutureReturnValueIgnored")
-  protected void succeedAndResetUserPromise() {
-    userPromise.setSuccess();
-    userPromise = null;
-  }
-
   protected void addCredentialHeaders(HttpRequest request, URI uri) throws IOException {
     String userInfo = uri.getUserInfo();
     if (userInfo != null) {

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

@@ -170,27 +170,37 @@ private HttpRequest buildRequest(String path, String host) {
   }
 
   private void succeedAndReset(ChannelHandlerContext 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. In addition, if reset
+    // calls ctx.close(), then that triggers a call to AbstractHttpHandler.channelInactive, which
+    // attempts to close the user promise.
+    ChannelPromise promise = userPromise;
+    userPromise = null;
     try {
-      succeedAndResetUserPromise();
-    } finally {
       reset(ctx);
+    } finally {
+      promise.setSuccess();
     }
   }
 
   @SuppressWarnings("FutureReturnValueIgnored")
   private void failAndClose(Throwable t, ChannelHandlerContext ctx) {
+    ChannelPromise promise = userPromise;
+    userPromise = null;
     try {
-      failAndResetUserPromise(t);
-    } finally {
       ctx.close();
+    } finally {
+      promise.setFailure(t);
     }
   }
 
   private void failAndReset(Throwable t, ChannelHandlerContext ctx) {
+    ChannelPromise promise = userPromise;
+    userPromise = null;
     try {
-      failAndResetUserPromise(t);
-    } finally {
       reset(ctx);
+    } finally {
+      promise.setFailure(t);
     }
   }
 

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

@@ -56,8 +56,17 @@ protected void channelRead0(ChannelHandlerContext ctx, FullHttpResponse response
       failAndClose(new IOException("Failed to parse the HTTP response."), ctx);
       return;
     }
+
+    checkState(userPromise != null, "response before request");
+    ChannelPromise promise = userPromise;
+    userPromise = null;
+    // 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.
     try {
-      checkState(userPromise != null, "response before request");
+      if (!HttpUtil.isKeepAlive(response)) {
+        ctx.close();
+      }
+    } finally {
       if (!response.status().equals(HttpResponseStatus.OK)
           && !response.status().equals(HttpResponseStatus.ACCEPTED)
           && !response.status().equals(HttpResponseStatus.CREATED)
@@ -69,13 +78,9 @@ protected void channelRead0(ChannelHandlerContext ctx, FullHttpResponse response
           response.content().readBytes(data);
           errorMsg += "\n" + new String(data, HttpUtil.getCharset(response));
         }
-        failAndResetUserPromise(new HttpException(response, errorMsg, null));
+        promise.setFailure(new HttpException(response, errorMsg, null));
       } else {
-        succeedAndResetUserPromise();
-      }
-    } finally {
-      if (!HttpUtil.isKeepAlive(response)) {
-        ctx.close();
+        promise.setSuccess();
       }
     }
   }
@@ -144,10 +149,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);
   }
 }

src/test/java/com/google/devtools/build/lib/remote/http/HttpUploadHandlerTest.java

@@ -40,7 +40,7 @@
 
 /** Tests for {@link HttpUploadHandler}. */
 @RunWith(JUnit4.class)
-public class HttpUploadHandlerTest extends AbstractHttpHandlerTest {
+public class HttpUploadHandlerTest {
 
   private static final URI CACHE_URI = URI.create("http://storage.googleapis.com:80/cache-bucket");