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.

Sign up for GitHub

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

Jump to bottom

netty:Fix Netty composite buffer merging to be compatible with Netty 4.1.111 #11294

Merged
merged 15 commits into from
Jun 20, 2024
Merged

netty:Fix Netty composite buffer merging to be compatible with Netty 4.1.111 #11294

Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8bde1c9
Change the logic for identifying changes to the read index when mergi…
larry-safran Jun 15, 2024
5353531
Change the logic for identifying changes to the read index when mergi…
larry-safran Jun 15, 2024
a87d7e7
Also use the old readerIndex, not just arrayOffset, to set the new re…
larry-safran Jun 15, 2024
294ceff
Fix debugging leftover
larry-safran Jun 15, 2024
5fd7d98
Fix mistaken file update
larry-safran Jun 15, 2024
b2cfffa
Utilize netty-buffer version number to choose approach to setting rea…
larry-safran Jun 15, 2024
40bb7f4
Improve version comparison to handle versions prior to 4.1.100
larry-safran Jun 17, 2024
2c18d7b
Change offset calculation logic to be based on the difference between…
larry-safran Jun 17, 2024
d8a8c44
Use the same approach for all netty versions.
larry-safran Jun 17, 2024
23de0bf
Use addComposite instead of addFlattenedComposite and not append to c…
larry-safran Jun 18, 2024
a36a797
Use addComposite instead of addFlattenedComposite and not append to c…
larry-safran Jun 18, 2024
4651e20
Add Javadoc
larry-safran Jun 18, 2024
6c8699f
Add Javadoc
larry-safran Jun 18, 2024
e941c01
Respond to review comments.
larry-safran Jun 20, 2024
7ff0d69
Replace most addFlattenedComponents with addComponent.
larry-safran Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
41 changes: 22 additions & 19 deletions netty/src/main/java/io/grpc/netty/NettyAdaptiveCumulator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,18 @@
import io.netty.buffer.ByteBufAllocator;
import io.netty.buffer.CompositeByteBuf;
import io.netty.handler.codec.ByteToMessageDecoder.Cumulator;
import io.netty.util.Version;

class NettyAdaptiveCumulator implements Cumulator {
private static final boolean usingPre4_1_111_Netty;
private final int composeMinSize;

static {
Version version = Version.identify().get("netty-buffer");
usingPre4_1_111_Netty =
version != null && version.artifactVersion().compareTo("4.1.111.Final") < 0;
larry-safran marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* "Adaptive" cumulator: cumulate {@link ByteBuf}s by dynamically switching between merge and
* compose strategies.
Expand Down Expand Up @@ -155,25 +163,20 @@ static void mergeWithCompositeTail(
// Take ownership of the tail.
newTail = tail.retain();

// TODO(https://github.com/netty/netty/issues/12844): remove when we use Netty with
ejona86 marked this conversation as resolved.
Show resolved Hide resolved
// the issue fixed.
// In certain cases, removing the CompositeByteBuf component, and then adding it back
// isn't idempotent. An example is provided in https://github.com/netty/netty/issues/12844.
// This happens because the buffer returned by composite.component() has out-of-sync
// indexes. Under the hood the CompositeByteBuf returns a duplicate() of the underlying
// buffer, but doesn't set the indexes.
//
// To get the right indexes we use the fact that composite.internalComponent() returns
// the slice() into the readable portion of the underlying buffer.
// We use this implementation detail (internalComponent() returning a *SlicedByteBuf),
// and combine it with the fact that SlicedByteBuf duplicates have their indexes
// adjusted so they correspond to the to the readable portion of the slice.
//
// Hence composite.internalComponent().duplicate() returns a buffer with the
// indexes that should've been on the composite.component() in the first place.
// Until the issue is fixed, we manually adjust the indexes of the removed component.
ByteBuf sliceDuplicate = composite.internalComponent(tailComponentIndex).duplicate();
newTail.setIndex(sliceDuplicate.readerIndex(), sliceDuplicate.writerIndex());
// In older versions some buffers don't support arrayOffset(), so for performance
// we don't want to rely on the arrayOffset when we know the alternate path works
if (usingPre4_1_111_Netty) {
// Because of the way duplicate() works in older netty versions this works. It stops
// working in 4.1.111 because they changed what duplicate returns.
ByteBuf sliceDuplicate = composite.internalComponent(tailComponentIndex).duplicate();
newTail.setIndex(sliceDuplicate.readerIndex(), sliceDuplicate.writerIndex());
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's fair to put blame on changes in Netty. It seems that this might have always been violating the API contract of Netty's CompositeByteBuf. The javadoc of CompositeByteBuf.internalComponent says that "updating the indexes of the returned buffer will lead to an undefined behavior of this buffer". That's exactly what happened here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lhotari No, it isn't updating the indexes of the internalComponent buffer, it is using values retrieved from the internalComponent to update the readerIndex of the buffer returned by composite.component. You can see netty/netty#12844 for a description of why this is being done. Note that this has been in place and working for a couple of years.

If you know of a better approach in netty 4 for appending to the existing last buffer in a composite than the approach we are taking, I'd be very interested in it.

} else {
int arrayOffset = composite.internalComponent(tailComponentIndex).arrayOffset();
if (arrayOffset > 0 && arrayOffset + tail.readerIndex() < tailSize) {
// The tail is a slice of a larger buffer, so we need to adjust the read index.
newTail.setIndex(arrayOffset + tail.readerIndex(), tail.writerIndex());
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.arrayOffset() is only available for heap ByteBufs (when hasArray() returns true). Are the components of the composite always heap ByteBufs?

The javadoc of CompositeByteBuf.internalComponent says that "updating the indexes of the returned buffer will lead to an undefined behavior of this buffer".
Is this solution attempting to update the indexes?

}

/*
* The tail is a readable non-composite buffer, so writeBytes() handles everything for us.
Expand Down