Repository: asterixdb Updated Branches: refs/heads/master 44f203e8c -> a0c1e1ad3
[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]> Project: http://git-wip-us.apache.org/repos/asf/asterixdb/repo Commit: http://git-wip-us.apache.org/repos/asf/asterixdb/commit/a0c1e1ad Tree: http://git-wip-us.apache.org/repos/asf/asterixdb/tree/a0c1e1ad Diff: http://git-wip-us.apache.org/repos/asf/asterixdb/diff/a0c1e1ad Branch: refs/heads/master Commit: a0c1e1ad390d5d12b0df4198efe873a6e6856276 Parents: 44f203e Author: Murtadha Hubail <[email protected]> Authored: Mon Oct 29 01:06:35 2018 +0300 Committer: Murtadha Hubail <[email protected]> Committed: Mon Oct 29 11:48:51 2018 -0700 ---------------------------------------------------------------------- .../hyracks/http/server/ChunkedResponse.java | 11 +++------- .../hyracks/http/server/FullResponse.java | 23 ++++---------------- .../hyracks/http/server/HttpServerHandler.java | 22 +++++++++++++------ .../hyracks/http/server/utils/HttpUtil.java | 8 +++++++ .../hyracks/http/test/HttpServerTest.java | 2 +- 5 files changed, 31 insertions(+), 35 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/asterixdb/blob/a0c1e1ad/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ChunkedResponse.java ---------------------------------------------------------------------- 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.OutputStream; 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.FullHttpRequest; 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 @@ public class ChunkedResponse implements IServletResponse { 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 @@ public class ChunkedResponse implements IServletResponse { 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 http://git-wip-us.apache.org/repos/asf/asterixdb/blob/a0c1e1ad/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/FullResponse.java ---------------------------------------------------------------------- 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.OutputStream; 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 @@ public class FullResponse implements IServletResponse { 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 http://git-wip-us.apache.org/repos/asf/asterixdb/blob/a0c1e1ad/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpServerHandler.java ---------------------------------------------------------------------- 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.Level; 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 @@ public class HttpServerHandler<T extends HttpServer> extends SimpleChannelInboun } } 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 @@ public class HttpServerHandler<T extends HttpServer> extends SimpleChannelInboun 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 @@ public class HttpServerHandler<T extends HttpServer> extends SimpleChannelInboun if (LOGGER.isDebugEnabled()) { LOGGER.debug("No servlet for " + request.uri()); } - respond(ctx, request.protocolVersion(), HttpResponseStatus.NOT_FOUND); + respond(ctx, request, HttpResponseStatus.NOT_FOUND); } @Override http://git-wip-us.apache.org/repos/asf/asterixdb/blob/a0c1e1ad/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/utils/HttpUtil.java ---------------------------------------------------------------------- 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.api.IServletResponse; 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 @@ public class HttpUtil { 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); + } } http://git-wip-us.apache.org/repos/asf/asterixdb/blob/a0c1e1ad/hyracks-fullstack/hyracks/hyracks-http/src/test/java/org/apache/hyracks/http/test/HttpServerTest.java ---------------------------------------------------------------------- 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 @@ public class HttpServerTest { 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();
