review comments

ywangd · Oct 28, 2024 · 885433d · 885433d
1 parent 90c0f50
commit 885433d
Show file tree

Hide file tree

Showing 2 changed files with 12 additions and 12 deletions.
diff --git a/...nalClusterTest/java/org/elasticsearch/http/netty4/Netty4IncrementalRequestHandlingIT.java b/...nalClusterTest/java/org/elasticsearch/http/netty4/Netty4IncrementalRequestHandlingIT.java
@@ -633,9 +633,8 @@ static class ServerRequestHandler implements BaseRestHandler.RequestBodyChunkCon
         public void handleChunk(RestChannel channel, ReleasableBytesReference chunk, boolean isLast) {
             Transports.assertTransportThread();
             if (shouldThrowInsideHandleChunk) {
-                if (randomBoolean()) {
-                    chunk.close();
-                }
+                // Must close the chunk. This is the contract of this method.
+                chunk.close();
                 throw new RuntimeException("simulated exception inside handleChunk");
             }
             recvChunks.add(new Chunk(chunk, isLast));

diff --git a/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java b/server/src/main/java/org/elasticsearch/rest/BaseRestHandler.java
@@ -128,15 +128,7 @@ public final void handleRequest(RestRequest request, RestChannel channel, NodeCl
                 request.contentStream().setHandler(new HttpBody.ChunkHandler() {
                     @Override
                     public void onNext(ReleasableBytesReference chunk, boolean isLast) {
-                        try {
-                            chunkConsumer.handleChunk(channel, chunk, isLast);
-                        } catch (Exception e) {
-                            // Release the chunk if the handler fails before releasing it in exceptional cases
-                            if (chunk.hasReferences()) {
-                                chunk.decRef();
-                            }
-                            throw e;
-                        }
+                        chunkConsumer.handleChunk(channel, chunk, isLast);
                     }
 
                     @Override
@@ -218,6 +210,15 @@ default void close() {}
     }
 
     public interface RequestBodyChunkConsumer extends RestChannelConsumer {
+
+        /**
+         * Handle one chunk of the request body. The handler <b>must</b> close the chunk once it is no longer
+         * needed to avoid leaking.
+         *
+         * @param channel The rest channel associated to the request
+         * @param chunk The chunk of request body that is ready for processing
+         * @param isLast Whether the chunk is the last one of the request
+         */
         void handleChunk(RestChannel channel, ReleasableBytesReference chunk, boolean isLast);
 
         /**