netty: Eliminate buffer release when there is an error as caller stil…

…l owns the buffer. (#10537) * netty: Eliminate buffer release when there is an error as caller still owns the buffer. --------- Co-authored-by: Sergii Tkachenko <hi@sergii.org>
grpc · Sep 15, 2023 · 6f09466 · 6f09466
1 parent e1334ea
commit 6f09466
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 12 deletions.
diff --git a/netty/src/main/java/io/grpc/netty/NettyAdaptiveCumulator.java b/netty/src/main/java/io/grpc/netty/NettyAdaptiveCumulator.java
@@ -204,16 +204,11 @@ static void mergeWithCompositeTail(
       composite.addFlattenedComponents(true, newTail);
       // New tail's ownership transferred to the composite buf.
       newTail = null;
-      in.release();
-      in = null;
-      // Restore the reader. In case it fails we restore the reader after releasing/forgetting
-      // the input and the new tail so that finally block can handles them properly.
       composite.readerIndex(prevReader);
+      // Input buffer was successfully merged with the tail.
+      // Must be the last line in the try because we release it only on success.
+      in.release();
     } finally {
-      // Input buffer was merged with the tail.
-      if (in != null) {
-        in.release();
-      }
       // If new tail's ownership isn't transferred to the composite buf.
       // Release it to prevent a leak.
       if (newTail != null) {

diff --git a/netty/src/test/java/io/grpc/netty/NettyAdaptiveCumulatorTest.java b/netty/src/test/java/io/grpc/netty/NettyAdaptiveCumulatorTest.java
@@ -533,18 +533,22 @@ public CompositeByteBuf addFlattenedComponents(boolean increaseWriterIndex,
 
       try {
         NettyAdaptiveCumulator.mergeWithCompositeTail(alloc, compositeThrows, in);
+        in = null; // On success it would be released
         fail("Cumulator didn't throw");
       } catch (UnsupportedOperationException actualError) {
         assertSame(expectedError, actualError);
-        // Input must be released unless its ownership has been to the composite cumulation.
-        assertEquals(0, in.refCnt());
+        // Because of error, ownership shouldn't have changed so should not have been released.
+        assertEquals(1, in.refCnt());
         // Tail released
         assertEquals(0, tail.refCnt());
         // Composite cumulation is retained
         assertEquals(1, compositeThrows.refCnt());
         // Composite cumulation loses the tail
         assertEquals(0, compositeThrows.numComponents());
       } finally {
+        if (in != null) {
+          in.release();
+        }
         compositeThrows.release();
       }
     }
@@ -569,18 +573,22 @@ public CompositeByteBuf addFlattenedComponents(boolean increaseWriterIndex,
 
       try {
         NettyAdaptiveCumulator.mergeWithCompositeTail(mockAlloc, compositeRo, in);
+        in = null; // On success it would be released
         fail("Cumulator didn't throw");
       } catch (UnsupportedOperationException actualError) {
         assertSame(expectedError, actualError);
-        // Input must be released unless its ownership has been to the composite cumulation.
-        assertEquals(0, in.refCnt());
+        // Because of error, ownership shouldn't have changed so should not have been released.
+        assertEquals(1, in.refCnt());
         // New buffer released
         assertEquals(0, newTail.refCnt());
         // Composite cumulation is retained
         assertEquals(1, compositeRo.refCnt());
         // Composite cumulation loses the tail
         assertEquals(0, compositeRo.numComponents());
       } finally {
+        if (in != null) {
+          in.release();
+        }
         compositeRo.release();
       }
     }