From 8e5a30a340000de0cf6ed16848513d962dfbd461 Mon Sep 17 00:00:00 2001 From: Ulf Adams Date: Fri, 22 Feb 2019 10:57:30 +0100 Subject: [PATCH] Reverse order of closing and completing the promise May fix #7464. --- .../blobstore/http/AbstractHttpHandler.java | 21 +++++----- .../blobstore/http/HttpDownloadHandler.java | 38 ++++++++----------- .../blobstore/http/HttpUploadHandler.java | 33 +++++++++------- 3 files changed, 44 insertions(+), 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 index 7b294987d73942..d8ffdef0294899 100644 --- 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 index bdb8d8fa869747..e2711715931995 100644 --- 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 index b3429789c50a06..4a8559ae062e65 100644 --- 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)); } }