Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Request chunks emitted after response is completed are not released #2592

Closed
romain-grecourt opened this issue Dec 9, 2020 · 1 comment · Fixed by #2605
Closed

Request chunks emitted after response is completed are not released #2592

romain-grecourt opened this issue Dec 9, 2020 · 1 comment · Fixed by #2605
Assignees
Labels
2.x Issues for 2.x version branch bug Something isn't working P1 webserver
Milestone

Comments

@romain-grecourt
Copy link
Contributor

romain-grecourt commented Dec 9, 2020

Request chunks emitted in the content publisher after the response is completed are not released, this triggers LEAK warning messages.

Any endpoint that returns a response instantly would reproduce this behavior.
Note that this is a regression introduced by #1830, and was originally fixed in #912

Reproduce

Any endpoint that returns a response without consuming its payload.
The example below invokes System.gc() to expose the LEAK warning message.

WebServer.builder(
        Routing.builder()
               .get((req, res) -> {
                   System.gc();
                   res.send("OK");
               })
               .build())
         .port(8080)
         .build()
         .start();

The chunks are emitted after routing, so this requires two invocation to trigger the LEAK warning message.

Invoke the endpoint twice:

curl -sSf http://localhost:8080/health -X GET -H "Content-Type: application/json" -d {}
curl -sSf http://localhost:8080/health -X GET -H "Content-Type: application/json" -d {}

Any HTTP method can be used, GET is only used as an example, but PUT or POST would work as well.

In the logs you should see:

WARNING: LEAK: RequestChunk.release() was not called before it was garbage collected. While the Reactive WebServer is designed to automatically release all the RequestChunks, it still comes with a considerable performance penalty and a demand for a large memory space (depending on expected throughput, it might require even more than 2GB). As such the users are strongly advised to release all the RequestChunk instances explicitly when they're not needed.

Fix

The following changes fix the issue:

diff --git a/webserver/webserver/src/main/java/io/helidon/webserver/ForwardingHandler.java b/webserver/webserver/src/main/java/io/helidon/webserver/ForwardingHandler.java
index e72bf391a..6e967f745 100644
--- a/webserver/webserver/src/main/java/io/helidon/webserver/ForwardingHandler.java
+++ b/webserver/webserver/src/main/java/io/helidon/webserver/ForwardingHandler.java
@@ -21,6 +21,7 @@ import java.util.Iterator;
 import java.util.Map;
 import java.util.Optional;
 import java.util.Queue;
+import java.util.concurrent.Flow;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.logging.Logger;
@@ -196,7 +197,25 @@ public class ForwardingHandler extends SimpleChannelInboundHandler<Object> {
                             if (queue.release()) {
                                 queues.remove(queue);
                             }
-                            publisherRef.clearBuffer(DataChunk::release);
+                            publisherRef.subscribe(new Flow.Subscriber<DataChunk>() {
+                                @Override
+                                public void onSubscribe(Flow.Subscription subscription) {
+                                    subscription.request(Long.MAX_VALUE);
+                                }
+
+                                @Override
+                                public void onNext(DataChunk item) {
+                                    item.release();
+                                }
+
+                                @Override
+                                public void onError(Throwable throwable) {
+                                }
+
+                                @Override
+                                public void onComplete() {
+                                }
+                            });
                             // Enable auto-read only after response has been completed
                             // to avoid a race condition with the next response
@romain-grecourt romain-grecourt added the 2.x Issues for 2.x version branch label Dec 9, 2020
@romain-grecourt romain-grecourt added this to the 2.2.0 milestone Dec 9, 2020
@romain-grecourt
Copy link
Contributor Author

@danielkec I've assigned this to you as there may be some subtle changes that are required.
Let me know if you need more details / or assistance.

@romain-grecourt romain-grecourt added webserver 1.x Issues for 1.x version branch bug Something isn't working labels Dec 9, 2020
@barchetta barchetta modified the milestones: 2.2.0, 1.4.8 Dec 9, 2020
@romain-grecourt romain-grecourt removed the 1.x Issues for 1.x version branch label Dec 9, 2020
@m0mus m0mus added P2 P1 and removed P2 labels Dec 10, 2020
danielkec added a commit to danielkec/helidon that referenced this issue Dec 14, 2020
Signed-off-by: Daniel Kec <daniel.kec@oracle.com>
danielkec added a commit to danielkec/helidon that referenced this issue Dec 14, 2020
Signed-off-by: Daniel Kec <daniel.kec@oracle.com>
danielkec added a commit that referenced this issue Dec 17, 2020
* Request chunks release fix #2592 with second subscribscribe attempt

Signed-off-by: Daniel Kec <daniel.kec@oracle.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.x Issues for 2.x version branch bug Something isn't working P1 webserver
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants