Skip to content

netty: create adaptive cumulator - #7532

Merged
sergiitk merged 5 commits into
grpc:masterfrom
sergiitk:netty-adaptive-cumulator
Nov 17, 2020
Merged

sergiitk merged 5 commits into
grpc:masterfrom
sergiitk:netty-adaptive-cumulator

Conversation

@sergiitk

@sergiitk sergiitk commented Oct 19, 2020 •

Copy link
Copy Markdown
Member

Creates "Adaptive" cumulator: cumulate ByteBuf's by dynamically switching between merge and compose strategies.

This cumulator applies a heuristic to make a decision whether to track a reference to the buffer with bytes received from the network stack in an array ("zero-copy"), or to merge into the last component (the tail) by performing a memory copy.

It is necessary as a protection from a potential attack on the COMPOSITE_CUMULATOR. Consider a pathological case when an attacker sends TCP packages containing a single byte of data, and forcing the cumulator to track each one in a separate buffer. In this case we'll be paying a memory overhead for each buffer, as well as extra compute to read the cumulation.

Implemented heuristic establishes a minimal threshold for the total size of the tail and incoming buffer, below which they are merged. The sum of the tail and the incoming buffer is used to avoid a case where attacker alternates the size of data packets to trick the cumulator into always selecting compose strategy.

Merging strategy attempts to minimize unnecessary memory writes. When possible, it expands the tail capacity and only copies the incoming buffer into available memory. Otherwise, when both tail and the buffer must be copied, the tail is reallocated (or fully replaced) with a new buffer of exponentially increasing capacity (bounded to minComposeSize) to ensure runtime O(n^2) amortized to O(n).

@sergiitk
sergiitk force-pushed the netty-adaptive-cumulator branch from 860ee7f to 0be7b25 Compare October 19, 2020 16:15
@sergiitk
sergiitk force-pushed the netty-adaptive-cumulator branch from 0be7b25 to 218f772 Compare October 19, 2020 16:30
@sergiitk
sergiitk marked this pull request as ready for review October 19, 2020 16:31
@sergiitk
sergiitk requested a review from ejona86 October 26, 2020 17:05
@ejona86

ejona86 commented Oct 30, 2020

Copy link
Copy Markdown
Member

FYI @Scottmitch @njhill . Note that this opens up opportunities for larger frame sizes. We would be interested to getting this into Netty, but we were going to be trying things out first.

@normanmaurer

Copy link
Copy Markdown

This sounds interesting... keep us posted

Comment thread netty/src/main/java/io/grpc/netty/NettyAdaptiveCumulator.java Outdated
Comment thread netty/src/test/java/io/grpc/netty/NettyTestUtil.java Outdated
};

// Throws an error on adding incoming buffer.
throwingCumulator = new NettyAdaptiveCumulator(0) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: normally this sort of thing we'd just do inline at the declaration and not in the @Before. This is fine though.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll get it fixed separately to not delay this.

@sergiitk
sergiitk merged commit 729175c into grpc:master Nov 17, 2020
@sergiitk
sergiitk deleted the netty-adaptive-cumulator branch November 17, 2020 23:05
@sergiitk
sergiitk restored the netty-adaptive-cumulator branch November 18, 2020 20:52
@sergiitk
sergiitk deleted the netty-adaptive-cumulator branch November 18, 2020 20:53
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Jun 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants