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

larry-safran · 2024-06-15T00:19:06Z

Change the logic for identifying changes to the read index when merging into a netty composite buffer so that it works with both older versions of netty and netty 4.1.111

Fixes #11284

…ng into a netty composite buffer so that it works with both older versions of netty and netty

…aderIndex

…derIndex during merges.

netty/src/main/java/io/grpc/netty/NettyAdaptiveCumulator.java

lhotari · 2024-06-17T06:06:36Z

netty/src/main/java/io/grpc/netty/NettyAdaptiveCumulator.java

+          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());
+          }


.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?

lhotari · 2024-06-17T06:09:57Z

netty/src/main/java/io/grpc/netty/NettyAdaptiveCumulator.java

+          // 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());


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.

@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.

… internal and external buffers' writeIndex values.

…omponents that are composites.

ejona86

The approach for this change is "CompositeByteBuf.addFlattenComponents() is broken, so stop using it." Only when using addFlattenComponents() do the components' readable area not equal the Composite's capacity.

netty/src/main/java/io/grpc/netty/NettyAdaptiveCumulator.java

sergiitk

Great job figuring out this solution, @larry-safran!

netty/src/main/java/io/grpc/netty/NettyAdaptiveCumulator.java

sergiitk · 2024-06-19T01:09:21Z

netty/src/main/java/io/grpc/netty/NettyAdaptiveCumulator.java

+ * compose strategies.
+ * <br><br>
+ *
+ * <p><b><font color="red">Avoid using</font></b>


I'm not sure this phrasing in red font makes a lot of sense here. It's not actionable for the user of this class. If it's for modifying the class itself, should we just note that on mergeWithCompositeTail method instead?

It isn't just for mergeWithCompositeTail. It is for the entire class. So cumulate() and addTail are impacted as well.

netty/src/test/java/io/grpc/netty/NettyAdaptiveCumulatorTest.java

idelpivnitskiy · 2024-06-21T17:29:07Z

@ejona86 @larry-safran I see you added "backport" and "release blocker" tags. Could you please share in what versions do you plan to backport this fix and what is ETA for new releases?

larry-safran · 2024-06-21T18:45:43Z

I plan to backport to 1.65 (not yet released), 1.64 & 1.63

…

On Fri, Jun 21, 2024 at 10:29 AM Idel Pivnitskiy ***@***.***> wrote: @ejona86 <https://github.com/ejona86> @larry-safran <https://github.com/larry-safran> I see you added "backport" and "release blocker" tags. Could you please share in what versions do you plan to backport this fix and what is ETA for new releases? — Reply to this email directly, view it on GitHub <#11294 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZQMCXTKL2CKOXBT3DDJCULZIRPHTAVCNFSM6AAAAABJLFX722VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBTGE2TONRWG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

…4.1.111 (grpc#11294) * Use addComponent instead of addFlattenedComponent and do not append to components that are composites.

…4.1.111 (#11294) (#11304) * Use addComponent instead of addFlattenedComponent and do not append to components that are composites.

…4.1.111 (#11294) (#11303) * Use addComponent instead of addFlattenedComponent and do not append to components that are composites.

…4.1.111 (#11294) (#11302) * Use addComponent instead of addFlattenedComponent and do not append to components that are composites.

idelpivnitskiy · 2024-06-25T16:59:56Z

@larry-safran what is ETA for 1.64.1 to be available in Maven Central?

larry-safran · 2024-06-25T18:01:54Z

I'm actively working on the patch release so it should be available this week.

…

On Tue, Jun 25, 2024 at 10:00 AM Idel Pivnitskiy ***@***.***> wrote: @larry-safran <https://github.com/larry-safran> what is ETA for 1.64.1 to be available in Maven Central? — Reply to this email directly, view it on GitHub <#11294 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZQMCXQ3ZZT6TEEESZUU7GTZJGO2HAVCNFSM6AAAAABJLFX722VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOBZGQ3TCNJXGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

…h Netty 4.1.111 (grpc#11294)" This reverts commit 0fea7dd, which broke internal benchmarks (b/350059169).

larry-safran added 2 commits June 14, 2024 17:11

Change the logic for identifying changes to the read index when mergi…

8bde1c9

…ng into a netty composite buffer so that it works with both older versions of netty and netty

Change the logic for identifying changes to the read index when mergi…

5353531

…ng into a netty composite buffer so that it works with both older versions of netty and netty

larry-safran marked this pull request as ready for review June 15, 2024 00:26

larry-safran requested a review from sergiitk June 15, 2024 00:26

larry-safran changed the title ~~Fix Netty composite buffer merging to be compatible with Netty~~ netty:Fix Netty composite buffer merging to be compatible with Netty Jun 15, 2024

larry-safran changed the title ~~netty:Fix Netty composite buffer merging to be compatible with Netty~~ netty:Fix Netty composite buffer merging to be compatible with Netty 4.1.111 Jun 15, 2024

larry-safran added the TODO:backport PR needs to be backported. Removed after backport complete label Jun 15, 2024

larry-safran added 4 commits June 14, 2024 18:14

Also use the old readerIndex, not just arrayOffset, to set the new re…

a87d7e7

…aderIndex

Fix debugging leftover

294ceff

Fix mistaken file update

5fd7d98

Utilize netty-buffer version number to choose approach to setting rea…

b2cfffa

…derIndex during merges.

panchenko reviewed Jun 15, 2024

View reviewed changes

netty/src/main/java/io/grpc/netty/NettyAdaptiveCumulator.java Outdated Show resolved Hide resolved

lhotari reviewed Jun 17, 2024

View reviewed changes

larry-safran added 3 commits June 17, 2024 11:08

Improve version comparison to handle versions prior to 4.1.100

40bb7f4

Change offset calculation logic to be based on the difference between…

2c18d7b

… internal and external buffers' writeIndex values.

Use the same approach for all netty versions.

d8a8c44

ejona86 added the TODO:release blocker Issue/PR is important enough to delay the release. Removed after release issues resolved label Jun 17, 2024

larry-safran added 2 commits June 18, 2024 10:14

Use addComposite instead of addFlattenedComposite and not append to c…

23de0bf

…omponents that are composites.

Use addComposite instead of addFlattenedComposite and not append to c…

a36a797

…omponents that are composites.

larry-safran requested review from ejona86 June 18, 2024 17:20

ejona86 approved these changes Jun 18, 2024

View reviewed changes

netty/src/main/java/io/grpc/netty/NettyAdaptiveCumulator.java Show resolved Hide resolved

larry-safran added 2 commits June 18, 2024 13:58

Add Javadoc

4651e20

Add Javadoc

6c8699f

ejona86 approved these changes Jun 18, 2024

View reviewed changes

sergiitk approved these changes Jun 19, 2024

View reviewed changes

sergiitk reviewed Jun 19, 2024

View reviewed changes

netty/src/test/java/io/grpc/netty/NettyAdaptiveCumulatorTest.java Show resolved Hide resolved

larry-safran added 2 commits June 20, 2024 14:06

Respond to review comments.

e941c01

Replace most addFlattenedComponents with addComponent.

7ff0d69

larry-safran merged commit 0fea7dd into grpc:master Jun 20, 2024
13 checks passed

larry-safran added a commit that referenced this pull request Jun 21, 2024

netty:Fix Netty composite buffer merging to be compatible with Netty …

c58eb96

…4.1.111 (#11294) (#11304) * Use addComponent instead of addFlattenedComponent and do not append to components that are composites.

larry-safran added a commit that referenced this pull request Jun 21, 2024

netty:Fix Netty composite buffer merging to be compatible with Netty …

f261200

…4.1.111 (#11294) (#11303) * Use addComponent instead of addFlattenedComponent and do not append to components that are composites.

larry-safran added a commit that referenced this pull request Jun 21, 2024

netty:Fix Netty composite buffer merging to be compatible with Netty …

b9927b0

…4.1.111 (#11294) (#11302) * Use addComponent instead of addFlattenedComponent and do not append to components that are composites.

larry-safran deleted the i_11284 branch June 21, 2024 23:53

ejona86 removed TODO:backport PR needs to be backported. Removed after backport complete TODO:release blocker Issue/PR is important enough to delay the release. Removed after release issues resolved labels Jun 24, 2024

zhangkun83 added a commit to zhangkun83/grpc-java that referenced this pull request Jul 9, 2024

Revert "netty:Fix Netty composite buffer merging to be compatible wit…

dc21092

…h Netty 4.1.111 (grpc#11294)" This reverts commit 0fea7dd, which broke internal benchmarks (b/350059169).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

larry-safran commented Jun 15, 2024 •

edited

Loading

lhotari Jun 17, 2024

lhotari Jun 17, 2024

larry-safran Jun 17, 2024

ejona86 left a comment

sergiitk left a comment

sergiitk Jun 19, 2024

ejona86 Jun 20, 2024

idelpivnitskiy commented Jun 21, 2024

larry-safran commented Jun 21, 2024 via email

idelpivnitskiy commented Jun 25, 2024

larry-safran commented Jun 25, 2024 via email

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

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

Conversation

larry-safran commented Jun 15, 2024 • edited Loading

lhotari Jun 17, 2024

Choose a reason for hiding this comment

lhotari Jun 17, 2024

Choose a reason for hiding this comment

larry-safran Jun 17, 2024

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

sergiitk left a comment

Choose a reason for hiding this comment

sergiitk Jun 19, 2024

Choose a reason for hiding this comment

ejona86 Jun 20, 2024

Choose a reason for hiding this comment

idelpivnitskiy commented Jun 21, 2024

larry-safran commented Jun 21, 2024 via email

idelpivnitskiy commented Jun 25, 2024

larry-safran commented Jun 25, 2024 via email

larry-safran commented Jun 15, 2024 •

edited

Loading