Repository: asterixdb Updated Branches: refs/heads/master 86f5a9edc -> 6de3315d3
[NO ISSUE][HTTP] Fix buffer leak in HttpServer - user model changes: no - storage format changes: no - interface changes: yes Details: - Prior to this change, cancelled requests before they start leak request and response buffers. - After this change, we distinguish between cancellation of requests before they start or after and release resources accordingly. Change-Id: I9a34142e87158385152fa0a11be39abced307fcc Reviewed-on: https://asterix-gerrit.ics.uci.edu/2901 Reviewed-by: Michael Blow <mb...@apache.org> Sonar-Qube: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Tested-by: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Contrib: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Integration-Tests: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Project: http://git-wip-us.apache.org/repos/asf/asterixdb/repo Commit: http://git-wip-us.apache.org/repos/asf/asterixdb/commit/6de3315d Tree: http://git-wip-us.apache.org/repos/asf/asterixdb/tree/6de3315d Diff: http://git-wip-us.apache.org/repos/asf/asterixdb/diff/6de3315d Branch: refs/heads/master Commit: 6de3315d3b65757ad95338823de687be421945be Parents: 86f5a9e Author: Abdullah Alamoudi <bamou...@gmail.com> Authored: Tue Aug 14 11:47:33 2018 -0700 Committer: abdullah alamoudi <bamou...@gmail.com> Committed: Tue Aug 14 16:50:10 2018 -0700 ---------------------------------------------------------------------- .../hyracks/http/api/IServletResponse.java | 5 +++++ .../http/server/ChunkedNettyOutputStream.java | 5 +++++ .../hyracks/http/server/ChunkedResponse.java | 5 +++++ .../hyracks/http/server/FullResponse.java | 5 +++++ .../hyracks/http/server/HttpRequestHandler.java | 22 +++++++++++++++++++- 5 files changed, 41 insertions(+), 1 deletion(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/asterixdb/blob/6de3315d/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/api/IServletResponse.java ---------------------------------------------------------------------- diff --git a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/api/IServletResponse.java b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/api/IServletResponse.java index 38f2d23..95f8f27 100644 --- a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/api/IServletResponse.java +++ b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/api/IServletResponse.java @@ -86,4 +86,9 @@ public interface IServletResponse extends Closeable { * Notifies the response that the channel has become inactive. */ void notifyChannelInactive(); + + /** + * Called on a created request that is cancelled before it is started + */ + void cancel(); } http://git-wip-us.apache.org/repos/asf/asterixdb/blob/6de3315d/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ChunkedNettyOutputStream.java ---------------------------------------------------------------------- diff --git a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ChunkedNettyOutputStream.java b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ChunkedNettyOutputStream.java index 891cc2a..adea133 100644 --- a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ChunkedNettyOutputStream.java +++ b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ChunkedNettyOutputStream.java @@ -29,6 +29,7 @@ import io.netty.buffer.ByteBuf; import io.netty.channel.ChannelHandlerContext; import io.netty.handler.codec.http.DefaultHttpContent; import io.netty.handler.codec.http.HttpResponseStatus; +import io.netty.util.ReferenceCountUtil; import io.netty.util.internal.OutOfDirectMemoryError; public class ChunkedNettyOutputStream extends OutputStream { @@ -137,4 +138,8 @@ public class ChunkedNettyOutputStream extends OutputStream { public synchronized void channelWritabilityChanged() { notifyAll(); } + + public void cancel() { + ReferenceCountUtil.release(buffer); + } } \ No newline at end of file http://git-wip-us.apache.org/repos/asf/asterixdb/blob/6de3315d/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 cd746b1..f02654e 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 @@ -192,4 +192,9 @@ public class ChunkedResponse implements IServletResponse { public void notifyChannelInactive() { outputStream.channelWritabilityChanged(); } + + @Override + public void cancel() { + outputStream.cancel(); + } } http://git-wip-us.apache.org/repos/asf/asterixdb/blob/6de3315d/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 90e33b6..1d28472 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 @@ -111,4 +111,9 @@ public class FullResponse implements IServletResponse { // Do nothing. // This response is sent as a single piece } + + @Override + public void cancel() { + // Do nothing, as this response doesn't allocate buffers in constructor + } } http://git-wip-us.apache.org/repos/asf/asterixdb/blob/6de3315d/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpRequestHandler.java ---------------------------------------------------------------------- diff --git a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpRequestHandler.java b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpRequestHandler.java index 72a8ea0..1c0801c 100644 --- a/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpRequestHandler.java +++ b/hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/HttpRequestHandler.java @@ -40,6 +40,8 @@ public class HttpRequestHandler implements Callable<Void> { private final IServlet servlet; private final IServletRequest request; private final IServletResponse response; + private boolean started = false; + private boolean cancelled = false; public HttpRequestHandler(ChannelHandlerContext ctx, IServlet servlet, IServletRequest request, int chunkSize) { this.ctx = ctx; @@ -52,6 +54,13 @@ public class HttpRequestHandler implements Callable<Void> { @Override public Void call() throws Exception { + synchronized (this) { + if (cancelled) { + LOGGER.warn("Request cancelled before it is started"); + return null; + } + started = true; + } try { ChannelFuture lastContentFuture = handle(); if (!HttpUtil.isKeepAlive(request.getHttpRequest())) { @@ -83,7 +92,18 @@ public class HttpRequestHandler implements Callable<Void> { } public void notifyChannelInactive() { - response.notifyChannelInactive(); + synchronized (this) { + if (!started) { + cancelled = true; + } + } + if (cancelled) { + // release request and response + response.cancel(); + request.getHttpRequest().release(); + } else { + response.notifyChannelInactive(); + } } public void reject() throws IOException {
