abdullah alamoudi has submitted this change and it was merged. Change subject: [NO ISSUE][HTTP] Fix buffer leak in HttpServer ......................................................................
[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> --- M hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/api/IServletResponse.java M hyracks-fullstack/hyracks/hyracks-http/src/main/java/org/apache/hyracks/http/server/ChunkedNettyOutputStream.java 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/HttpRequestHandler.java 5 files changed, 41 insertions(+), 1 deletion(-) Approvals: Anon. E. Moose #1000171: Jenkins: Verified; No violations found; ; Verified Michael Blow: Looks good to me, approved 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 @@ * 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(); } 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.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 synchronized void channelWritabilityChanged() { notifyAll(); } + + public void cancel() { + ReferenceCountUtil.release(buffer); + } } \ No newline at end of file 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 void notifyChannelInactive() { outputStream.channelWritabilityChanged(); } + + @Override + public void cancel() { + outputStream.cancel(); + } } 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 @@ // 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 + } } 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 @@ 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 @@ @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 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 { -- To view, visit https://asterix-gerrit.ics.uci.edu/2901 To unsubscribe, visit https://asterix-gerrit.ics.uci.edu/settings Gerrit-MessageType: merged Gerrit-Change-Id: I9a34142e87158385152fa0a11be39abced307fcc Gerrit-PatchSet: 5 Gerrit-Project: asterixdb Gerrit-Branch: master Gerrit-Owner: abdullah alamoudi <bamou...@gmail.com> Gerrit-Reviewer: Anon. E. Moose #1000171 Gerrit-Reviewer: Ian Maxon <ima...@apache.org> Gerrit-Reviewer: Jenkins <jenk...@fulliautomatix.ics.uci.edu> Gerrit-Reviewer: Michael Blow <mb...@apache.org> Gerrit-Reviewer: Murtadha Hubail <mhub...@apache.org> Gerrit-Reviewer: Till Westmann <ti...@apache.org> Gerrit-Reviewer: abdullah alamoudi <bamou...@gmail.com>
