Repository: netty/netty
Issue: CompositeByteBuf.addComponent ownership clarification
Labels: defect

If all goes well `CompositeByteBuf.addComponent` will take "ownership" (will call `release()`) of the parameter. However there are cases where this is not the case. For example if the `CompositeByteBuf` has been released `addComponent` may throw an `IllegalReferenceCountException` and not release the parameter. At the very least the interface should be clarified so that the caller can understand expectations. However I would like see if there are any objections to `CompositeByteBuf.addComponent` always assuming "ownership" of the parameter. This would be analogous to a `Channel.write` always assuming responsibility of the parameter even if the underlying transport cannot actually support the write operation.


Issue comments:
/cc @normanmaurer @nmittler @trustin 

@trustin ping again


Pull request review comments:
unused import ?

ReferenceCountUtil.safeRelease(...) ?

see above

I was just anticipating your next comments :wink: 

I originally had this, but figured we could avoid the instanceof check and casting by the simple `try/catch` block.


Patch that resolved the issue:
diff --git a/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java b/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
index 262b229c327..be2ce7917f5 100644
--- a/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
+++ b/buffer/src/main/java/io/netty/buffer/CompositeByteBuf.java
@@ -33,6 +33,8 @@
 import java.util.ListIterator;
 import java.util.NoSuchElementException;
 
+import static io.netty.util.internal.ObjectUtil.checkNotNull;
+
 /**
  * A virtual buffer which shows multiple buffers as a single merged buffer.  It is recommended to use
  * {@link ByteBufAllocator#compositeBuffer()} or {@link Unpooled#wrappedBuffer(ByteBuf...)} instead of calling the
@@ -117,13 +119,16 @@ private static List<Component> newList(int maxNumComponents) {
 
     /**
      * Add the given {@link ByteBuf}.
-     *
+     * <p>
      * Be aware that this method does not increase the {@code writerIndex} of the {@link CompositeByteBuf}.
      * If you need to have it increased you need to handle it by your own.
-     *
-     * @param buffer the {@link ByteBuf} to add
+     * <p>
+     * {@link ByteBuf#release()} ownership of {@code buffer} is transfered to this {@link CompositeByteBuf}.
+     * @param buffer the {@link ByteBuf} to add. {@link ByteBuf#release()} ownership is transfered to this
+     * {@link CompositeByteBuf}.
      */
     public CompositeByteBuf addComponent(ByteBuf buffer) {
+        checkNotNull(buffer, "buffer");
         addComponent0(components.size(), buffer);
         consolidateIfNeeded();
         return this;
@@ -131,11 +136,14 @@ public CompositeByteBuf addComponent(ByteBuf buffer) {
 
     /**
      * Add the given {@link ByteBuf}s.
-     *
+     * <p>
      * Be aware that this method does not increase the {@code writerIndex} of the {@link CompositeByteBuf}.
      * If you need to have it increased you need to handle it by your own.
-     *
-     * @param buffers the {@link ByteBuf}s to add
+     * <p>
+     * {@link ByteBuf#release()} ownership of all {@link ByteBuf} objects in {@code buffers} is transfered to this
+     * {@link CompositeByteBuf}.
+     * @param buffers the {@link ByteBuf}s to add. {@link ByteBuf#release()} ownership of all {@link ByteBuf#release()}
+     * ownership of all {@link ByteBuf} objects is transfered to this {@link CompositeByteBuf}.
      */
     public CompositeByteBuf addComponents(ByteBuf... buffers) {
         addComponents0(components.size(), buffers);
@@ -145,11 +153,14 @@ public CompositeByteBuf addComponents(ByteBuf... buffers) {
 
     /**
      * Add the given {@link ByteBuf}s.
-     *
+     * <p>
      * Be aware that this method does not increase the {@code writerIndex} of the {@link CompositeByteBuf}.
      * If you need to have it increased you need to handle it by your own.
-     *
-     * @param buffers the {@link ByteBuf}s to add
+     * <p>
+     * {@link ByteBuf#release()} ownership of all {@link ByteBuf} objects in {@code buffers} is transfered to this
+     * {@link CompositeByteBuf}.
+     * @param buffers the {@link ByteBuf}s to add. {@link ByteBuf#release()} ownership of all {@link ByteBuf#release()}
+     * ownership of all {@link ByteBuf} objects is transfered to this {@link CompositeByteBuf}.
      */
     public CompositeByteBuf addComponents(Iterable<ByteBuf> buffers) {
         addComponents0(components.size(), buffers);
@@ -159,56 +170,73 @@ public CompositeByteBuf addComponents(Iterable<ByteBuf> buffers) {
 
     /**
      * Add the given {@link ByteBuf} on the specific index.
-     *
+     * <p>
      * Be aware that this method does not increase the {@code writerIndex} of the {@link CompositeByteBuf}.
      * If you need to have it increased you need to handle it by your own.
-     *
-     * @param cIndex the index on which the {@link ByteBuf} will be added
-     * @param buffer the {@link ByteBuf} to add
+     * <p>
+     * {@link ByteBuf#release()} ownership of {@code buffer} is transfered to this {@link CompositeByteBuf}.
+     * @param cIndex the index on which the {@link ByteBuf} will be added.
+     * @param buffer the {@link ByteBuf} to add. {@link ByteBuf#release()} ownership is transfered to this
+     * {@link CompositeByteBuf}.
      */
     public CompositeByteBuf addComponent(int cIndex, ByteBuf buffer) {
+        checkNotNull(buffer, "buffer");
         addComponent0(cIndex, buffer);
         consolidateIfNeeded();
         return this;
     }
 
+    /**
+     * Precondition is that {@code buffer != null}.
+     */
     private int addComponent0(int cIndex, ByteBuf buffer) {
-        checkComponentIndex(cIndex);
-
-        if (buffer == null) {
-            throw new NullPointerException("buffer");
-        }
+        assert buffer != null;
+        boolean wasAdded = false;
+        try {
+            checkComponentIndex(cIndex);
 
-        int readableBytes = buffer.readableBytes();
+            int readableBytes = buffer.readableBytes();
 
-        // No need to consolidate - just add a component to the list.
-        Component c = new Component(buffer.order(ByteOrder.BIG_ENDIAN).slice());
-        if (cIndex == components.size()) {
-            components.add(c);
-            if (cIndex == 0) {
-                c.endOffset = readableBytes;
+            // No need to consolidate - just add a component to the list.
+            @SuppressWarnings("deprecation")
+            Component c = new Component(buffer.order(ByteOrder.BIG_ENDIAN).slice());
+            if (cIndex == components.size()) {
+                wasAdded = components.add(c);
+                if (cIndex == 0) {
+                    c.endOffset = readableBytes;
+                } else {
+                    Component prev = components.get(cIndex - 1);
+                    c.offset = prev.endOffset;
+                    c.endOffset = c.offset + readableBytes;
+                }
             } else {
-                Component prev = components.get(cIndex - 1);
-                c.offset = prev.endOffset;
-                c.endOffset = c.offset + readableBytes;
+                components.add(cIndex, c);
+                wasAdded = true;
+                if (readableBytes != 0) {
+                    updateComponentOffsets(cIndex);
+                }
             }
-        } else {
-            components.add(cIndex, c);
-            if (readableBytes != 0) {
-                updateComponentOffsets(cIndex);
+            return cIndex;
+        } finally {
+            if (!wasAdded) {
+                buffer.release();
             }
         }
-        return cIndex;
     }
 
     /**
      * Add the given {@link ByteBuf}s on the specific index
-     *
+     * <p>
      * Be aware that this method does not increase the {@code writerIndex} of the {@link CompositeByteBuf}.
      * If you need to have it increased you need to handle it by your own.
-     *
-     * @param cIndex the index on which the {@link ByteBuf} will be added.
-     * @param buffers the {@link ByteBuf}s to add
+     * <p>
+     * {@link ByteBuf#release()} ownership of all {@link ByteBuf} objects in {@code buffers} is transfered to this
+     * {@link CompositeByteBuf}.
+     * @param cIndex the index on which the {@link ByteBuf} will be added. {@link ByteBuf#release()} ownership of all
+     * {@link ByteBuf#release()} ownership of all {@link ByteBuf} objects is transfered to this
+     * {@link CompositeByteBuf}.
+     * @param buffers the {@link ByteBuf}s to add. {@link ByteBuf#release()} ownership of all {@link ByteBuf#release()}
+     * ownership of all {@link ByteBuf} objects is transfered to this {@link CompositeByteBuf}.
      */
     public CompositeByteBuf addComponents(int cIndex, ByteBuf... buffers) {
         addComponents0(cIndex, buffers);
@@ -217,24 +245,38 @@ public CompositeByteBuf addComponents(int cIndex, ByteBuf... buffers) {
     }
 
     private int addComponents0(int cIndex, ByteBuf... buffers) {
-        checkComponentIndex(cIndex);
-
-        if (buffers == null) {
-            throw new NullPointerException("buffers");
-        }
-
-        // No need for consolidation
-        for (ByteBuf b: buffers) {
-            if (b == null) {
-                break;
+        checkNotNull(buffers, "buffers");
+        int i = 0;
+        try {
+            checkComponentIndex(cIndex);
+
+            // No need for consolidation
+            while (i < buffers.length) {
+                // Increment i now to prepare for the next iteration and prevent a duplicate release (addComponent0
+                // will release if an exception occurs, and we also release in the finally block here).
+                ByteBuf b = buffers[i++];
+                if (b == null) {
+                    break;
+                }
+                cIndex = addComponent0(cIndex, b) + 1;
+                int size = components.size();
+                if (cIndex > size) {
+                    cIndex = size;
+                }
             }
-            cIndex = addComponent0(cIndex, b) + 1;
-            int size = components.size();
-            if (cIndex > size) {
-                cIndex = size;
+            return cIndex;
+        } finally {
+            for (; i < buffers.length; ++i) {
+                ByteBuf b = buffers[i];
+                if (b != null) {
+                    try {
+                        b.release();
+                    } catch (Throwable ignored) {
+                        // ignore
+                    }
+                }
             }
         }
-        return cIndex;
     }
 
     /**
@@ -242,9 +284,13 @@ private int addComponents0(int cIndex, ByteBuf... buffers) {
      *
      * Be aware that this method does not increase the {@code writerIndex} of the {@link CompositeByteBuf}.
      * If you need to have it increased you need to handle it by your own.
-     *
+     * <p>
+     * {@link ByteBuf#release()} ownership of all {@link ByteBuf} objects in {@code buffers} is transfered to this
+     * {@link CompositeByteBuf}.
      * @param cIndex the index on which the {@link ByteBuf} will be added.
-     * @param buffers the {@link ByteBuf}s to add
+     * @param buffers the {@link ByteBuf}s to add.  {@link ByteBuf#release()} ownership of all
+     * {@link ByteBuf#release()} ownership of all {@link ByteBuf} objects is transfered to this
+     * {@link CompositeByteBuf}.
      */
     public CompositeByteBuf addComponents(int cIndex, Iterable<ByteBuf> buffers) {
         addComponents0(cIndex, buffers);
@@ -253,21 +299,32 @@ public CompositeByteBuf addComponents(int cIndex, Iterable<ByteBuf> buffers) {
     }
 
     private int addComponents0(int cIndex, Iterable<ByteBuf> buffers) {
-        if (buffers == null) {
-            throw new NullPointerException("buffers");
-        }
-
         if (buffers instanceof ByteBuf) {
             // If buffers also implements ByteBuf (e.g. CompositeByteBuf), it has to go to addComponent(ByteBuf).
             return addComponent0(cIndex, (ByteBuf) buffers);
         }
+        checkNotNull(buffers, "buffers");
 
         if (!(buffers instanceof Collection)) {
             List<ByteBuf> list = new ArrayList<ByteBuf>();
-            for (ByteBuf b: buffers) {
-                list.add(b);
+            try {
+                for (ByteBuf b: buffers) {
+                    list.add(b);
+                }
+                buffers = list;
+            } finally {
+                if (buffers != list) {
+                    for (ByteBuf b: buffers) {
+                        if (b != null) {
+                            try {
+                                b.release();
+                            } catch (Throwable ignored) {
+                                // ignore
+                            }
+                        }
+                    }
+                }
             }
-            buffers = list;
         }
 
         Collection<ByteBuf> col = (Collection<ByteBuf>) buffers;
@@ -1457,10 +1514,7 @@ public CompositeByteBuf discardReadBytes() {
     }
 
     private ByteBuf allocBuffer(int capacity) {
-        if (direct) {
-            return alloc().directBuffer(capacity);
-        }
-        return alloc().heapBuffer(capacity);
+        return direct ? alloc().directBuffer(capacity) : alloc().heapBuffer(capacity);
     }
 
     @Override
@@ -1482,7 +1536,6 @@ private static final class Component {
         }
 
         void freeIfNecessary() {
-            // Unwrap so that we can free slices, too.
             buf.release(); // We should not get a NPE here. If so, it must be a bug.
         }
     }


Test patch: