[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 <jenkins@fulliautomatix.ics.uci.edu>
Tested-by: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Contrib: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Integration-Tests: Jenkins <jenkins@fulliautomatix.ics.uci.edu>
Reviewed-by: Murtadha Hubail <mhubail@apache.org>
Reviewed-by: Michael Blow <mblow@apache.org>
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();