Murtadha Hubail has submitted this change and it was merged. Change subject: [NO ISSUE][OTH] Do Not Close Client Connection After Failure ......................................................................
[NO ISSUE][OTH] Do Not Close Client Connection After Failure - user model changes: no - storage format changes: no - interface changes: no Details: - Currently, after sending some failure responses (e.g. after servlet not found), the client connection is closed even if the connection was supposed to be kept alive. This change ensures that we do not close the client connection -- if keep alive is requested -- which allows the client to submit another request using the same connection. - Ensure a full http response is sent to the client in case of a failure and not only the response header. - Refactor logic to set connection header. Change-Id: Id0fce2c860eec97f3d368ee42f25dbdfc9dc0ff9 Reviewed-on: https://asterix-gerrit.ics.uci.edu/3006 Sonar-Qube: Jenkins <[email protected]> Tested-by: Jenkins <[email protected]> Contrib: Jenkins <[email protected]> Integration-Tests: Jenkins <[email protected]> Reviewed-by: Murtadha Hubail <[email protected]> Reviewed-by: Michael Blow <[email protected]> --- M hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ChunkedResponse.java M hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/FullResponse.java M hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServerHandler.java M hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/utils/HttpUtil.java M hyracks-fullstack/hyracks/hyracks-http/src/test/java/org/apache/hyracks/http/test/HttpServerTest.java 5 files changed, 31 insertions(+), 35 deletions(-) Approvals: Anon. E. Moose #1000171: Jenkins: Verified; No violations found; ; Verified Michael Blow: Looks good to me, approved Murtadha Hubail: Looks good to me, but someone else must approve diff --git a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ChunkedResponse.java b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ChunkedResponse.java index a67b40e..b3a7587 100644 --- a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ChunkedResponse.java +++ b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ChunkedResponse.java @@ -23,13 +23,13 @@ import java.io.PrintWriter; import org.apache.hyracks.http.api.IServletResponse; +import org.apache.hyracks.http.server.utils.HttpUtil; import org.apache.logging.log4j.Level; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import io.netty.buffer.ByteBuf; import io.netty.channel.ChannelFuture; -import io.netty.channel.ChannelFutureListener; import io.netty.channel.ChannelHandlerContext; import io.netty.handler.codec.http.DefaultFullHttpResponse; import io.netty.handler.codec.http.DefaultHttpResponse; @@ -37,9 +37,7 @@ import io.netty.handler.codec.http.HttpHeaderNames; import io.netty.handler.codec.http.HttpHeaderValues; import io.netty.handler.codec.http.HttpHeaders; -import io.netty.handler.codec.http.HttpResponse; import io.netty.handler.codec.http.HttpResponseStatus; -import io.netty.handler.codec.http.HttpUtil; import io.netty.handler.codec.http.HttpVersion; import io.netty.handler.codec.http.LastHttpContent; @@ -65,12 +63,11 @@ private final ChannelHandlerContext ctx; private final ChunkedNettyOutputStream outputStream; private final PrintWriter writer; - private HttpResponse response; + private DefaultHttpResponse response; private boolean headerSent; private ByteBuf error; private ChannelFuture future; private boolean done; - private final boolean keepAlive; public ChunkedResponse(ChannelHandlerContext ctx, FullHttpRequest request, int chunkSize) { this.ctx = ctx; @@ -78,9 +75,7 @@ writer = new PrintWriter(outputStream); response = new DefaultHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.INTERNAL_SERVER_ERROR); response.headers().set(HttpHeaderNames.TRANSFER_ENCODING, HttpHeaderValues.CHUNKED); - keepAlive = HttpUtil.isKeepAlive(request); - response.headers().set(HttpHeaderNames.CONNECTION, - keepAlive ? HttpHeaderValues.KEEP_ALIVE : HttpHeaderValues.CLOSE); + HttpUtil.setConnectionHeader(request, response); } @Override diff --git a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/FullResponse.java b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/FullResponse.java index 1d28472..55bbd30 100644 --- a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/FullResponse.java +++ b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/FullResponse.java @@ -24,26 +24,23 @@ import java.io.PrintWriter; import org.apache.hyracks.http.api.IServletResponse; +import org.apache.hyracks.http.server.utils.HttpUtil; import io.netty.buffer.Unpooled; import io.netty.channel.ChannelFuture; -import io.netty.channel.ChannelFutureListener; import io.netty.channel.ChannelHandlerContext; import io.netty.handler.codec.http.DefaultFullHttpResponse; import io.netty.handler.codec.http.FullHttpRequest; import io.netty.handler.codec.http.FullHttpResponse; import io.netty.handler.codec.http.HttpHeaderNames; -import io.netty.handler.codec.http.HttpHeaderValues; import io.netty.handler.codec.http.HttpResponseStatus; -import io.netty.handler.codec.http.HttpUtil; import io.netty.handler.codec.http.HttpVersion; public class FullResponse implements IServletResponse { private final ChannelHandlerContext ctx; private final ByteArrayOutputStream baos; private final PrintWriter writer; - private final FullHttpResponse response; - private final boolean keepAlive; + private final DefaultFullHttpResponse response; private ChannelFuture future; public FullResponse(ChannelHandlerContext ctx, FullHttpRequest request) { @@ -51,27 +48,15 @@ baos = new ByteArrayOutputStream(); writer = new PrintWriter(baos); response = new DefaultFullHttpResponse(HttpVersion.HTTP_1_1, HttpResponseStatus.INTERNAL_SERVER_ERROR); - keepAlive = HttpUtil.isKeepAlive(request); - if (keepAlive) { - response.headers().set(HttpHeaderNames.CONNECTION, HttpHeaderValues.KEEP_ALIVE); - } + HttpUtil.setConnectionHeader(request, response); } @Override public void close() throws IOException { writer.close(); FullHttpResponse fullResponse = response.replace(Unpooled.copiedBuffer(baos.toByteArray())); - if (keepAlive) { - if (response.status() == HttpResponseStatus.OK || response.status() == HttpResponseStatus.UNAUTHORIZED) { - fullResponse.headers().setInt(HttpHeaderNames.CONTENT_LENGTH, fullResponse.content().readableBytes()); - } else { - fullResponse.headers().remove(HttpHeaderNames.CONNECTION); - } - } + fullResponse.headers().setInt(HttpHeaderNames.CONTENT_LENGTH, fullResponse.content().readableBytes()); future = ctx.writeAndFlush(fullResponse); - if (response.status() != HttpResponseStatus.OK && response.status() != HttpResponseStatus.UNAUTHORIZED) { - future.addListener(ChannelFutureListener.CLOSE); - } } @Override diff --git a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServerHandler.java b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServerHandler.java index baa664a..36d79f3 100644 --- a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServerHandler.java +++ b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServerHandler.java @@ -30,13 +30,16 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import io.netty.channel.ChannelFuture; import io.netty.channel.ChannelFutureListener; import io.netty.channel.ChannelHandlerContext; import io.netty.channel.SimpleChannelInboundHandler; +import io.netty.handler.codec.http.DefaultFullHttpResponse; import io.netty.handler.codec.http.DefaultHttpResponse; import io.netty.handler.codec.http.FullHttpRequest; +import io.netty.handler.codec.http.HttpHeaderNames; +import io.netty.handler.codec.http.HttpRequest; import io.netty.handler.codec.http.HttpResponseStatus; -import io.netty.handler.codec.http.HttpVersion; public class HttpServerHandler<T extends HttpServer> extends SimpleChannelInboundHandler<Object> { @@ -92,13 +95,18 @@ } } catch (Exception e) { LOGGER.log(Level.WARN, "Failure Submitting HTTP Request", e); - respond(ctx, request.protocolVersion(), new HttpResponseStatus(500, e.getMessage())); + respond(ctx, request, HttpResponseStatus.INTERNAL_SERVER_ERROR); } } - protected void respond(ChannelHandlerContext ctx, HttpVersion httpVersion, HttpResponseStatus status) { - DefaultHttpResponse response = new DefaultHttpResponse(httpVersion, status); - ctx.writeAndFlush(response).addListener(ChannelFutureListener.CLOSE); + protected void respond(ChannelHandlerContext ctx, HttpRequest request, HttpResponseStatus status) { + final DefaultHttpResponse response = new DefaultFullHttpResponse(request.protocolVersion(), status); + response.headers().setInt(HttpHeaderNames.CONTENT_LENGTH, 0); + HttpUtil.setConnectionHeader(request, response); + final ChannelFuture clientChannel = ctx.writeAndFlush(response); + if (!io.netty.handler.codec.http.HttpUtil.isKeepAlive(request)) { + clientChannel.addListener(ChannelFutureListener.CLOSE); + } } private void submit(ChannelHandlerContext ctx, IServlet servlet, FullHttpRequest request) throws IOException { @@ -107,7 +115,7 @@ servletRequest = HttpUtil.toServletRequest(request); } catch (IllegalArgumentException e) { LOGGER.log(Level.WARN, "Failure Decoding Request", e); - respond(ctx, request.protocolVersion(), HttpResponseStatus.BAD_REQUEST); + respond(ctx, request, HttpResponseStatus.BAD_REQUEST); return; } handler = new HttpRequestHandler(ctx, servlet, servletRequest, chunkSize); @@ -128,7 +136,7 @@ if (LOGGER.isDebugEnabled()) { LOGGER.debug("No servlet for " + request.uri()); } - respond(ctx, request.protocolVersion(), HttpResponseStatus.NOT_FOUND); + respond(ctx, request, HttpResponseStatus.NOT_FOUND); } @Override diff --git a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/utils/HttpUtil.java b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/utils/HttpUtil.java index 8d6dfbc..b0249fb 100644 --- a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/utils/HttpUtil.java +++ b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/utils/HttpUtil.java @@ -30,9 +30,12 @@ import org.apache.hyracks.http.server.BaseRequest; import org.apache.hyracks.http.server.FormUrlEncodedRequest; +import io.netty.handler.codec.http.DefaultHttpResponse; import io.netty.handler.codec.http.FullHttpRequest; import io.netty.handler.codec.http.HttpHeaderNames; +import io.netty.handler.codec.http.HttpHeaderValues; import io.netty.handler.codec.http.HttpRequest; +import io.netty.util.AsciiString; public class HttpUtil { private static final Pattern PARENT_DIR = Pattern.compile("/[^./]+/\\.\\./"); @@ -154,4 +157,9 @@ return clusterURL; } + public static void setConnectionHeader(HttpRequest request, DefaultHttpResponse response) { + final boolean keepAlive = io.netty.handler.codec.http.HttpUtil.isKeepAlive(request); + final AsciiString connectionHeaderValue = keepAlive ? HttpHeaderValues.KEEP_ALIVE : HttpHeaderValues.CLOSE; + response.headers().set(HttpHeaderNames.CONNECTION, connectionHeaderValue); + } } diff --git a/hyracks-fullstack/hyracks/hyracks-http/src/test/java/org/apache/hyracks/http/test/HttpServerTest.java b/hyracks-fullstack/hyracks/hyracks-http/src/test/java/org/apache/hyracks/http/test/HttpServerTest.java index 6b8b51a..c1d1315 100644 --- a/hyracks-fullstack/hyracks/hyracks-http/src/test/java/org/apache/hyracks/http/test/HttpServerTest.java +++ b/hyracks-fullstack/hyracks/hyracks-http/src/test/java/org/apache/hyracks/http/test/HttpServerTest.java @@ -240,7 +240,7 @@ pw.flush(); BufferedReader br = new BufferedReader(new InputStreamReader(s.getInputStream())); String line; - while ((line = br.readLine()) != null) { + while ((line = br.readLine()) != null && !line.isEmpty()) { response.append(line).append('\n'); } br.close(); -- To view, visit https://asterix-gerrit.ics.uci.edu/3006 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: merged Gerrit-Change-Id: Id0fce2c860eec97f3d368ee42f25dbdfc9dc0ff9 Gerrit-PatchSet: 5 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Jenkins <[email protected]> Gerrit-Reviewer: Michael Blow <[email protected]> Gerrit-Reviewer: Murtadha Hubail <[email protected]> Gerrit-Reviewer: Till Westmann <[email protected]>
