Fix for incorrect values from CompositeByteBuf#component(int)#9525
Merged
normanmaurer merged 3 commits intonetty:4.1from Sep 2, 2019
Merged
Fix for incorrect values from CompositeByteBuf#component(int)#9525normanmaurer merged 3 commits intonetty:4.1from
normanmaurer merged 3 commits intonetty:4.1from
Conversation
|
Can one of the admins verify this patch? |
Motivation This is a "simpler" alternative to netty#9416 which fixes the same CompositeByteBuf bugs described there, originally reported by @jingene in netty#9398. Modifications - Add fields to Component class for the original buffer along with its adjustment, which may be different to the already-stored unwrapped buffer. Use it in appropriate places to ensure correctness and equivalent behaviour to that prior to the earlier optimizations - Add comments explaining purpose of each of the Component fields - Unwrap more kinds of buffers in newComponent method to extend scope of the existing indirection-reduction optimization - De-duplicate common buffer consolidation logic - Unit test for the original bug provided by @jingene Result - Correct behaviour / fixed bugs - Some code deduplication / simplification - Unwrapping optimization applied to more types of buffers The downside is increased mem footprint from the two new fields, and additional allocations in some specific cases, though those should be rare. Co-authored-by: jingene <jingene0206@gmail.com>
3ed0b59 to
36a9041
Compare
Member
|
@netty-bot test this please |
Contributor
|
LGTM 👍 |
Member
|
@netty-bot test this please |
| @SuppressWarnings("deprecation") | ||
| private Component newComponent(final ByteBuf buf, final int offset) { | ||
| final int srcIndex = buf.readerIndex(); | ||
| final int len = buf.writerIndex() - buf.readerIndex(); |
Member
There was a problem hiding this comment.
nit: final int len = buf.readableBytes()
normanmaurer
requested changes
Aug 30, 2019
| return buf.duplicate().setIndex(idx(offset), idx(endOffset)); | ||
| // We can only use the cached slice safely if it's equal to the source buffer, | ||
| // otherwise it might not be fully visible to us | ||
| return (slice == srcBuf ? srcBuf : srcBuf.slice()).duplicate(); |
Member
There was a problem hiding this comment.
Why do we need to first slice and then also duplicate ?
Member
Author
There was a problem hiding this comment.
@normanmaurer oops, I had meant this to be .slice(srcIdx(offset), length()). But thinking some more and comparing further to the "original" impl I think you are right that srcBuf.duplicate() would be ok and obviously preferable overhead-wise.
I was concerned about the possibility of someone independently modifying the indices of a buffer after it was added, but we make it clear that the CompositeByteBuf takes "ownership" of all added buffers (mainly refcount-wise but I think indices would be assumed too).
Member
Author
|
Thanks @franz1981 @jingene @normanmaurer, updated to address comments |
Member
|
@netty-bot test this please |
normanmaurer
approved these changes
Sep 2, 2019
Member
normanmaurer
pushed a commit
that referenced
this pull request
Sep 2, 2019
Motivation This is a "simpler" alternative to #9416 which fixes the same CompositeByteBuf bugs described there, originally reported by @jingene in #9398. Modifications - Add fields to Component class for the original buffer along with its adjustment, which may be different to the already-stored unwrapped buffer. Use it in appropriate places to ensure correctness and equivalent behaviour to that prior to the earlier optimizations - Add comments explaining purpose of each of the Component fields - Unwrap more kinds of buffers in newComponent method to extend scope of the existing indirection-reduction optimization - De-duplicate common buffer consolidation logic - Unit test for the original bug provided by @jingene Result - Correct behaviour / fixed bugs - Some code deduplication / simplification - Unwrapping optimization applied to more types of buffers The downside is increased mem footprint from the two new fields, and additional allocations in some specific cases, though those should be rare. Co-authored-by: jingene <jingene0206@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
This is a "simpler" alternative to #9416 which fixes the same
CompositeByteBufbugs described there, originally reported by @jingene in #9398.Modifications
Componentclass for the original buffer along with its adjustment, which may be different to the already-stored unwrapped buffer. Use it in appropriate places to ensure correctness and equivalent behaviour to that prior to the earlier optimizationsComponentfieldsnewComponentmethod to extend scope of the existing indirection-reduction optimizationResult
The downside is increased mem footprint from the two new fields, and additional allocations in some specific cases, though those should be rare.
Co-authored-by: jingene jingene0206@gmail.com