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.

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

netty: create adaptive cumulator #7532

Merged
merged 5 commits into from
Nov 17, 2020

Conversation

sergiitk
Copy link
Member

@sergiitk sergiitk commented Oct 19, 2020

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 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
Copy link
Member

ejona86 commented Oct 30, 2020

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

This sounds interesting... keep us posted

@@ -84,6 +86,14 @@ void addInput(ByteBufAllocator alloc, CompositeByteBuf composite, ByteBuf in) {
composite.addFlattenedComponents(true, in);
}
};

// Throws an error on adding incoming buffer.
throwingCumulator = new NettyAdaptiveCumulator(0) {
Copy link
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
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.

None yet

3 participants