CompositeByteBuf.component() returns ByteBuf duplicates without adjusting the indexes

[sergiitk]

Expected behavior

Removing a component from a CompositeByteBuf, and adding it back to the same place does not change the content of the CompositeByteBuf.

Actual behavior

In certain cases, removing a component from the CompositeByteBuf, and adding it back results in the CompositeByteBuf with the content different from before the removal. In such cases, CompositeByteBuf.component() returns a buffer with the reader index out-of-sync with the readable portion of the buffer contributing to the CompositeByteBuf.

Steps to reproduce

This corresponds to the code example in the next section.

1. Create a new CompositeByteBuf with a single component, content: ---01234; read first 3 bytes:

###### composite1
Full:
         +-------------------------------------------------+
         |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f |
+--------+-------------------------------------------------+----------------+
|00000000| 2d 2d 2d 30 31 32 33 34                         |---01234        |
+--------+-------------------------------------------------+----------------+
Readable:
         +-------------------------------------------------+
         |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f |
+--------+-------------------------------------------------+----------------+
|00000000| 30 31 32 33 34                                  |01234           |
+--------+-------------------------------------------------+----------------+
CompositeByteBuf(ridx: 3, widx: 8, cap: 8, components=1)

2. Wrap it into another CompositeByteBuf:

###### composite2
Full:
         +-------------------------------------------------+
         |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f |
+--------+-------------------------------------------------+----------------+
|00000000| 30 31 32 33 34                                  |01234           |
+--------+-------------------------------------------------+----------------+
Readable:
         +-------------------------------------------------+
         |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f |
+--------+-------------------------------------------------+----------------+
|00000000| 30 31 32 33 34                                  |01234           |
+--------+-------------------------------------------------+----------------+
CompositeByteBuf(ridx: 0, widx: 5, cap: 5, components=1)

Note that now the full view of the buffer is the same as the readable portion, as addFlattenedComponents made the adjustments to exclude the bytes already read (---).

3. Let's explore the result of CompositeByteBuf.component(0):

###### component
Full:
         +-------------------------------------------------+
         |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f |
+--------+-------------------------------------------------+----------------+
|00000000| 2d 2d 2d 30 31 32 33 34                         |---01234        |
+--------+-------------------------------------------------+----------------+
Readable:
         +-------------------------------------------------+
         |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f |
+--------+-------------------------------------------------+----------------+
|00000000| 2d 2d 2d 30 31 32 33 34                         |---01234        |
+--------+-------------------------------------------------+----------------+
UnpooledDuplicatedByteBuf(ridx: 0, widx: 8, cap: 32, unwrapped: PooledUnsafeHeapByteBuf(ridx: 0, widx: 8, cap: 32))

Note that the component still contains the bytes discarded by the CompositeByteBuf(). This contradicts to what's returned by internalComponent(), which returns the slice() under the hood:

###### internalComponent
Full:
         +-------------------------------------------------+
         |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f |
+--------+-------------------------------------------------+----------------+
|00000000| 30 31 32 33 34                                  |01234           |
+--------+-------------------------------------------------+----------------+
Readable:
         +-------------------------------------------------+
         |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f |
+--------+-------------------------------------------------+----------------+
|00000000| 30 31 32 33 34                                  |01234           |
+--------+-------------------------------------------------+----------------+
UnpooledSlicedByteBuf(ridx: 0, widx: 5, cap: 5/5, unwrapped: PooledUnsafeHeapByteBuf(ridx: 0, widx: 8, cap: 32))

What's even more interesting, if we call .duplicate() on the slice, it'll return the full view of the buffer, and with correctly offset indexes!

###### internalComponentDup
Full:
         +-------------------------------------------------+
         |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f |
+--------+-------------------------------------------------+----------------+
|00000000| 2d 2d 2d 30 31 32 33 34                         |---01234        |
+--------+-------------------------------------------------+----------------+
Readable:
         +-------------------------------------------------+
         |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f |
+--------+-------------------------------------------------+----------------+
|00000000| 30 31 32 33 34                                  |01234           |
+--------+-------------------------------------------------+----------------+
UnpooledDuplicatedByteBuf(ridx: 3, widx: 8, cap: 32, unwrapped: PooledUnsafeHeapByteBuf(ridx: 0, widx: 8, cap: 32))

5. Remove the component, then add it back to the composite buf. Results:
   Actual

###### Resulting composite 
Full:
         +-------------------------------------------------+
         |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f |
+--------+-------------------------------------------------+----------------+
|00000000| 2d 2d 2d 30 31 32 33 34                         |---01234        |
+--------+-------------------------------------------------+----------------+
Readable:
         +-------------------------------------------------+
         |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f |
+--------+-------------------------------------------------+----------------+
|00000000| 2d 2d 2d 30 31 32 33 34                         |---01234        |
+--------+-------------------------------------------------+----------------+
CompositeByteBuf(ridx: 0, widx: 8, cap: 8, components=1)

Expected

###### Resulting composite 
Full:
         +-------------------------------------------------+
         |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f |
+--------+-------------------------------------------------+----------------+
|00000000| 30 31 32 33 34                                  |01234           |
+--------+-------------------------------------------------+----------------+
Readable:
         +-------------------------------------------------+
         |  0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f |
+--------+-------------------------------------------------+----------------+
|00000000| 30 31 32 33 34                                  |01234           |
+--------+-------------------------------------------------+----------------+
CompositeByteBuf(ridx: 0, widx: 5, cap: 5, components=1)

Minimal yet complete reproducer code

package io.grpc.netty;

import static com.google.common.truth.Truth.assertThat;
import static io.netty.util.CharsetUtil.US_ASCII;

import io.netty.buffer.ByteBuf;
import io.netty.buffer.ByteBufAllocator;
import io.netty.buffer.ByteBufUtil;
import io.netty.buffer.CompositeByteBuf;
import io.netty.buffer.PooledByteBufAllocator;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.JUnit4;

@RunWith(JUnit4.class)
public class CompositeByteBufTest {
  @Test
  public void compositeByteBuf_componentIndexes_demo() {
    ByteBufAllocator alloc = new PooledByteBufAllocator();

    // Create underlying buffer spacious enough for the test data.
    ByteBuf buf = alloc.buffer(32).writeBytes("---01234".getBytes(US_ASCII));

    // Start with a regular cumulation and add the buf as the only component.
    CompositeByteBuf composite1 = alloc.compositeBuffer(8).addFlattenedComponents(true, buf);
    // Read composite1 buf to the beginning of the numbers.
    assertThat(composite1.readCharSequence(3, US_ASCII).toString()).isEqualTo("---");

    printBufDebug("composite1", composite1);

    // Wrap composite1 into another CompositeByteBuf. This is similar to
    // what NettyAdaptiveCumulator.cumulate() does in the case the cumulation has refCnt != 1.
    CompositeByteBuf composite2 =
        alloc.compositeBuffer(8).addFlattenedComponents(true, composite1);
    assertThat(composite2.toString(US_ASCII)).isEqualTo("01234");

    printBufDebug("composite2", composite2);

    // The previous operation does not adjust the read indexes of the underlying buffers,
    // only the internal Component offsets.

    // Remove the last component from the cumulation, then add it back.
    int tailComponentIndex = composite2.numComponents() - 1;
    int tailStart = composite2.toByteIndex(tailComponentIndex);
    ByteBuf component = composite2.component(tailComponentIndex);
    printBufDebug("component", component);

    // internalComponent and internalComponentDup are for the demo purposes.
    // ByteBuf internalComponent = composite2.internalComponent(tailComponentIndex);
    // ByteBuf internalComponentDup = composite2.internalComponent(tailComponentIndex).duplicate();
    // printBufDebug("internalComponent", internalComponent);
    // printBufDebug("internalComponentDup", internalComponentDup);

    // Take the ownership of the component.
    component.retain();

    // Remove the component from the composite buf.
    composite2.removeComponent(tailComponentIndex).setIndex(0, tailStart);

    // Add it back to the composite byte buf.
    composite2.addFlattenedComponents(true, component);
    printBufDebug("Resulting composite ", composite2);

    assertThat(composite2.toString(US_ASCII)).isEqualTo("01234");
  }

  private static void printBufDebug(String title, ByteBuf buf) {
    String msg = "###### " + title + "\n";
    msg += "Full:\n";
    msg += ByteBufUtil.prettyHexDump(buf, 0, buf.readerIndex() + buf.readableBytes())  + "\n";
    msg += "Readable:\n";
    msg += ByteBufUtil.prettyHexDump(buf)  + "\n";
    msg += buf + "\n";
    System.out.println(msg);
  }
}

Netty version

4.x

JVM version (e.g. java -version)

Any

OS version (e.g. uname -a)

Any

Misc

This is the root cause for the initial rollback of NettyAdaptiveCumulator in gRPC (grpc/grpc-java#7532).
Now that we found the issue, we're bringing back the NettyAdaptiveCumulator in grpc/grpc-java#9558, but using a hack with copying the indexes from the internalComponent().duplicate(). This issue is to track the fix in the upstream.

cc @ejona86 @normanmaurer

[normanmaurer]

@chrisvest can you have a look ?

[chrisvest]

As I understand it, the essence of the issue is that CompositeByteBuf.component(int) returns a duplicate of the component as it was originally given, rather than as a projection through the composite buffer index ranges.

We can ask ourselves if getting a component buffer out of a composite buffer is the dual of adding, or if it's only the readable contents of the buffers that matters.

As far as I can see, tests and documentation don't say either way, but I'm hesitant to change this for compatibility.
Also, looking at other methods, none of them modify the indices of the component buffers, so there is precedence for leaving them alone.

We can update the javadoc to say that the component indices do not reflect the indices of the composite buffer.
We can also add a new method, "componentSlice" or something, which returns a slice of the component for the bytes-that-are-readable-through-the-composite-buffer, which I think is what you're after.

@sergiitk How does that sound? Would you be interested in contributing such a PR?

[sergiitk]

Hey @chrisvest. Yes, I was going to make the PR soonish. Regarding what approach to choose - I'll follow up later, need to dig a bit more first.