Merge pull request #784 from shintasmith/httpConnIdleTimeout

add logic to send back Keep-Alive header if httpConnectionIdleTimeout is non zero
rax-maas · Feb 20, 2017 · ddd6fea · ddd6fea
2 parents 3cbb88f + 592ff20
commit ddd6fea
Show file tree

Hide file tree

Showing 12 changed files with 111 additions and 17 deletions.
diff --git a/blueflood-http/src/main/java/com/rackspacecloud/blueflood/http/DefaultHandler.java b/blueflood-http/src/main/java/com/rackspacecloud/blueflood/http/DefaultHandler.java
@@ -43,9 +43,11 @@ public class DefaultHandler implements HttpRequestHandler {
 
     private static final Logger log = LoggerFactory.getLogger(DefaultHandler.class);
 
+    private static final HttpResponder responder = HttpResponder.getInstance();
+
     @Override
     public void handle(ChannelHandlerContext ctx, FullHttpRequest request) {
-        HttpResponder.respond(ctx, request, HttpResponseStatus.OK);
+        responder.respond(ctx, request, HttpResponseStatus.OK);
     }
 
     public static void sendErrorResponse(ChannelHandlerContext ctx, FullHttpRequest request,
@@ -98,7 +100,7 @@ public static void sendResponse(ChannelHandlerContext channel, FullHttpRequest r
                 response.content().writeBytes(Unpooled.copiedBuffer(messageBody, Constants.DEFAULT_CHARSET));
             }
 
-            HttpResponder.respond(channel, request, response);
+            responder.respond(channel, request, response);
             Tracker.getInstance().trackResponse(request, response);
         } finally {
             sendResponseTimerContext.stop();

diff --git a/blueflood-http/src/main/java/com/rackspacecloud/blueflood/http/HttpResponder.java b/blueflood-http/src/main/java/com/rackspacecloud/blueflood/http/HttpResponder.java
@@ -16,15 +16,14 @@
 
 package com.rackspacecloud.blueflood.http;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.rackspacecloud.blueflood.service.Configuration;
 import com.rackspacecloud.blueflood.service.CoreConfig;
+import com.rackspacecloud.blueflood.service.HttpConfig;
 import io.netty.buffer.Unpooled;
 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.HttpResponseStatus;
+import io.netty.handler.codec.http.*;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -38,14 +37,30 @@ public class HttpResponder {
 
     private static final boolean CORS_ENABLED = Configuration.getInstance().getBooleanProperty(CoreConfig.CORS_ENABLED);
     private static final String CORS_ALLOWED_ORIGINS = Configuration.getInstance().getStringProperty(CoreConfig.CORS_ALLOWED_ORIGINS);
+    private static final String KEEP_ALIVE_TIMEOUT_STR = "timeout=";
 
     private static final Logger log = LoggerFactory.getLogger(HttpResponder.class);
 
-    public static void respond(ChannelHandlerContext ctx, FullHttpRequest req, HttpResponseStatus status) {
+    private static HttpResponder INSTANCE = new HttpResponder();
+
+    public static HttpResponder getInstance() { return INSTANCE; }
+
+    private int httpConnIdleTimeout =
+            Configuration.getInstance().getIntegerProperty(HttpConfig.HTTP_CONNECTION_READ_IDLE_TIME_SECONDS);
+
+    @VisibleForTesting
+    HttpResponder() {}
+
+    @VisibleForTesting
+    HttpResponder(int httpConnIdleTimeout) {
+        this.httpConnIdleTimeout = httpConnIdleTimeout;
+    }
+
+    public void respond(ChannelHandlerContext ctx, FullHttpRequest req, HttpResponseStatus status) {
         respond(ctx, req, new DefaultFullHttpResponse(HTTP_1_1, status));
     }
 
-    public static void respond(ChannelHandlerContext ctx, FullHttpRequest req, FullHttpResponse res) {
+    public void respond(ChannelHandlerContext ctx, FullHttpRequest req, FullHttpResponse res) {
 
         // set response headers
         if (CORS_ENABLED) {
@@ -59,6 +74,13 @@ public static void respond(ChannelHandlerContext ctx, FullHttpRequest req, FullH
         boolean isKeepAlive = isKeepAlive(req);
         if (isKeepAlive) {
             res.headers().add(CONNECTION, KEEP_ALIVE);
+
+            // if this config is set to non zero, it means we intend
+            // to close this connection after x seconds. We need to
+            // respond back with the right headers to indicate this
+            if ( httpConnIdleTimeout > 0 ) {
+                res.headers().add(KEEP_ALIVE, KEEP_ALIVE_TIMEOUT_STR + httpConnIdleTimeout);
+            }
         }
 
         // Send the response and close the connection if necessary.

diff --git a/blueflood-http/src/main/java/com/rackspacecloud/blueflood/http/NoRouteHandler.java b/blueflood-http/src/main/java/com/rackspacecloud/blueflood/http/NoRouteHandler.java
@@ -24,6 +24,6 @@ public class NoRouteHandler implements HttpRequestHandler {
 
     @Override
     public void handle(ChannelHandlerContext context, FullHttpRequest request) {
-        HttpResponder.respond(context, request, HttpResponseStatus.NOT_FOUND);
+        HttpResponder.getInstance().respond(context, request, HttpResponseStatus.NOT_FOUND);
     }
 }
diff --git a/...ood-http/src/main/java/com/rackspacecloud/blueflood/http/QueryStringDecoderAndRouter.java b/...ood-http/src/main/java/com/rackspacecloud/blueflood/http/QueryStringDecoderAndRouter.java
@@ -79,7 +79,7 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable thr) {
         if (thr.getCause() instanceof TooLongFrameException) {
             // todo: meter these so we observe DOS conditions.
             log.warn(String.format("Long frame from %s", ctx.channel().remoteAddress()));
-            HttpResponder.respond(ctx, null, HttpResponseStatus.BAD_REQUEST);
+            HttpResponder.getInstance().respond(ctx, null, HttpResponseStatus.BAD_REQUEST);
         } else {
             log.warn(String.format("Exception event received from %s: ", ctx.channel().remoteAddress()), thr);
         }

diff --git a/blueflood-http/src/main/java/com/rackspacecloud/blueflood/http/UnsupportedMethodHandler.java b/blueflood-http/src/main/java/com/rackspacecloud/blueflood/http/UnsupportedMethodHandler.java
@@ -22,6 +22,7 @@
 import java.util.Set;
 
 public class UnsupportedMethodHandler implements HttpRequestHandler {
+
     private final RouteMatcher routeMatcher;
     private final FullHttpResponse response;
 
@@ -41,6 +42,6 @@ public void handle(ChannelHandlerContext context, FullHttpRequest request) {
         }
         final String methodsAllowed =  result.length() > 0 ? result.substring(0, result.length() - 1): "";
         response.headers().add("Allow", methodsAllowed);
-        HttpResponder.respond(context, request, response);
+        HttpResponder.getInstance().respond(context, request, response);
     }
 }
diff --git a/blueflood-http/src/main/java/com/rackspacecloud/blueflood/http/UnsupportedVerbsHandler.java b/blueflood-http/src/main/java/com/rackspacecloud/blueflood/http/UnsupportedVerbsHandler.java
@@ -24,6 +24,6 @@ public class UnsupportedVerbsHandler implements HttpRequestHandler {
 
     @Override
     public void handle(ChannelHandlerContext ctx, FullHttpRequest request) {
-        HttpResponder.respond(ctx, request, HttpResponseStatus.NOT_IMPLEMENTED);
+        HttpResponder.getInstance().respond(ctx, request, HttpResponseStatus.NOT_IMPLEMENTED);
     }
 }
diff --git a/.../src/main/java/com/rackspacecloud/blueflood/outputs/handlers/HttpMetricTokensHandler.java b/.../src/main/java/com/rackspacecloud/blueflood/outputs/handlers/HttpMetricTokensHandler.java
@@ -79,7 +79,7 @@ private void sendResponse(ChannelHandlerContext channel, FullHttpRequest request
             response.content().writeBytes(Unpooled.copiedBuffer(messageBody, Constants.DEFAULT_CHARSET));
         }
 
-        HttpResponder.respond(channel, request, response);
+        HttpResponder.getInstance().respond(channel, request, response);
         Tracker.getInstance().trackResponse(request, response);
     }
 

diff --git a/.../src/main/java/com/rackspacecloud/blueflood/outputs/handlers/HttpMetricsIndexHandler.java b/.../src/main/java/com/rackspacecloud/blueflood/outputs/handlers/HttpMetricsIndexHandler.java
@@ -67,7 +67,7 @@ private void sendResponse(ChannelHandlerContext channel, FullHttpRequest request
             response.content().writeBytes(Unpooled.copiedBuffer(messageBody, Constants.DEFAULT_CHARSET));
         }
 
-        HttpResponder.respond(channel, request, response);
+        HttpResponder.getInstance().respond(channel, request, response);
         Tracker.getInstance().trackResponse(request, response);
     }
 

diff --git a/...main/java/com/rackspacecloud/blueflood/outputs/handlers/HttpMultiRollupsQueryHandler.java b/...main/java/com/rackspacecloud/blueflood/outputs/handlers/HttpMultiRollupsQueryHandler.java
@@ -159,7 +159,7 @@ private void sendResponse(ChannelHandlerContext channel, FullHttpRequest request
             response.content().writeBytes(Unpooled.copiedBuffer(messageBody, Constants.DEFAULT_CHARSET));
         }
 
-        HttpResponder.respond(channel, request, response);
+        HttpResponder.getInstance().respond(channel, request, response);
         Tracker.getInstance().trackResponse(request, response);
     }
 }
diff --git a/.../src/main/java/com/rackspacecloud/blueflood/outputs/handlers/HttpRollupsQueryHandler.java b/.../src/main/java/com/rackspacecloud/blueflood/outputs/handlers/HttpRollupsQueryHandler.java
@@ -169,7 +169,7 @@ private void sendResponse(ChannelHandlerContext channel, FullHttpRequest request
             response.content().writeBytes(Unpooled.copiedBuffer(messageBody, Constants.DEFAULT_CHARSET));
         }
 
-        HttpResponder.respond(channel, request, response);
+        HttpResponder.getInstance().respond(channel, request, response);
         Tracker.getInstance().trackResponse(request, response);
     }
 }
diff --git a/blueflood-http/src/main/java/com/rackspacecloud/blueflood/service/HttpConfig.java b/blueflood-http/src/main/java/com/rackspacecloud/blueflood/service/HttpConfig.java
@@ -62,7 +62,7 @@ Maximum number of WORKER threads for HTTP output (is included in connections cal
     HTTP_MAX_TYPE_UNIT_PROCESSOR_THREADS("10"),
 
     // Idle time allowed on a connection, with no inbound traffic, before closing the connection. Specify 0 to disable.
-    HTTP_CONNECTION_READ_IDLE_TIME_SECONDS("120");
+    HTTP_CONNECTION_READ_IDLE_TIME_SECONDS("0");
 
     static {
         Configuration.getInstance().loadDefaults(HttpConfig.values());

diff --git a/blueflood-http/src/test/java/com/rackspacecloud/blueflood/http/HttpResponderTest.java b/blueflood-http/src/test/java/com/rackspacecloud/blueflood/http/HttpResponderTest.java
@@ -0,0 +1,69 @@
+package com.rackspacecloud.blueflood.http;
+
+import io.netty.channel.Channel;
+import io.netty.channel.ChannelFuture;
+import io.netty.channel.ChannelHandlerContext;
+import io.netty.handler.codec.http.*;
+import org.junit.Test;
+import static org.junit.Assert.*;
+import static org.mockito.Mockito.*;
+
+/**
+ *
+ */
+public class HttpResponderTest {
+
+    @Test
+    public void testDefaultHttpConnIdleTimeout_RequestKeepAlive_ShouldHaveResponseKeepAlive() {
+        HttpResponder responder = new HttpResponder();
+
+        ChannelHandlerContext ctx = mock(ChannelHandlerContext.class);
+        FullHttpRequest request = mock(FullHttpRequest.class);
+        FullHttpResponse response = mock(FullHttpResponse.class);
+        HttpHeaders headers = mock(HttpHeaders.class);
+        Channel channel = mock(Channel.class);
+
+        when(ctx.channel()).thenReturn(channel);
+
+        when(request.content()).thenReturn(null);
+        when(request.headers()).thenReturn(headers);
+        when(headers.get(HttpHeaders.Names.CONNECTION)).thenReturn(HttpHeaders.Values.KEEP_ALIVE);
+        when(request.getProtocolVersion()).thenReturn(HttpVersion.HTTP_1_1);
+
+        HttpHeaders responseHeaders = new DefaultHttpHeaders();
+        when(response.headers()).thenReturn(responseHeaders);
+
+        responder.respond(ctx, request, response);
+
+        assertEquals("Connection: response header", HttpHeaders.Values.KEEP_ALIVE, responseHeaders.get(HttpHeaders.Names.CONNECTION));
+    }
+
+    @Test
+    public void testNonZeroHttpConnIdleTimeout_RequestKeepAlive_ShouldHaveResponseKeepAliveTimeout() {
+        int idleTimeout = 300;
+        HttpResponder responder = new HttpResponder(idleTimeout);
+
+        ChannelHandlerContext ctx = mock(ChannelHandlerContext.class);
+        FullHttpRequest request = mock(FullHttpRequest.class);
+        FullHttpResponse response = mock(FullHttpResponse.class);
+        HttpHeaders headers = mock(HttpHeaders.class);
+        Channel channel = mock(Channel.class);
+        ChannelFuture future = mock(ChannelFuture.class);
+
+        when(ctx.channel()).thenReturn(channel);
+        when(ctx.writeAndFlush(any())).thenReturn(future);
+
+        when(request.content()).thenReturn(null);
+        when(request.headers()).thenReturn(headers);
+        when(headers.get(HttpHeaders.Names.CONNECTION)).thenReturn(HttpHeaders.Values.KEEP_ALIVE);
+        when(request.getProtocolVersion()).thenReturn(HttpVersion.HTTP_1_1);
+
+        HttpHeaders responseHeaders = new DefaultHttpHeaders();
+        when(response.headers()).thenReturn(responseHeaders);
+
+        responder.respond(ctx, request, response);
+
+        assertEquals("Connection: response header", HttpHeaders.Values.KEEP_ALIVE, responseHeaders.get(HttpHeaders.Names.CONNECTION));
+        assertEquals("Keep-Alive: response header", "timeout="+idleTimeout, responseHeaders.get("Keep-Alive"));
+    }
+}