Remove dangerous ByteBufStreamInput methods#27076
Merged
Tim-Brooks merged 3 commits intoelastic:masterfrom Oct 24, 2017
Merged
Conversation
This commit removes the `ByteBufStreamInput` `readBytesReference` and `readBytesRef` methods. These methods are zero-copy which means that they retain a reference to the underlying netty buffer. The problem is that our `TcpTransport` is not designed to handle zero-copy. The netty implementation sets the read index past the current message once it has been deserialized, handled, and mostly likely dispatched to another thread. This means that netty is free to release this buffer. So it is unsafe to retain a reference to it without calling `retain`. And we cannot call `retain` because we are not currently designed to handle reference counting past the transport level. This should not currently impact us as we wrap the `ByteBufStreamInput` in `NamedWriteableAwareStreamInput` in the `TcpTransport`. This stream essentially delegates to the underling stream. However, in the case of `readBytesReference` and `readBytesRef` it leaves thw implementations to the standard `StreamInput` methods. These methods call the read byte array method which delegates to `ByteBufStreamInput`. The read byte array method on `ByteBufStreamInput` copies so it is safe. The only impact of this commit should be removing methods that could be dangerous if they were eventually called due to some refactoring.
jpountz
approved these changes
Oct 23, 2017
Contributor
jpountz
left a comment
There was a problem hiding this comment.
LGTM. I left a suggestion about making it more future-proof.
| BytesRef bytesRef = new BytesRef(buffer.array(), buffer.arrayOffset() + buffer.readerIndex(), length); | ||
| buffer.skipBytes(length); | ||
| return bytesRef; | ||
| } |
Contributor
There was a problem hiding this comment.
should we keep them, delegating to the parent impl with a comment, eg.
@Override
public BytesReference readBytesReference(int length) throws IOException {
// NOTE: It is unsafe to share a reference of the internal structure, so we
// need to return a copy, like the default implementation does.
return super.readBytesReference(length);
}
My motivation is that it won't be possible to optimize it unsafely in the future without seeing that comment.
Tim-Brooks
added a commit
that referenced
this pull request
Oct 24, 2017
This commit removes the `ByteBufStreamInput` `readBytesReference` and `readBytesRef` methods. These methods are zero-copy which means that they retain a reference to the underlying netty buffer. The problem is that our `TcpTransport` is not designed to handle zero-copy. The netty implementation sets the read index past the current message once it has been deserialized, handled, and mostly likely dispatched to another thread. This means that netty is free to release this buffer. So it is unsafe to retain a reference to it without calling `retain`. And we cannot call `retain` because we are not currently designed to handle reference counting past the transport level. This should not currently impact us as we wrap the `ByteBufStreamInput` in `NamedWriteableAwareStreamInput` in the `TcpTransport`. This stream essentially delegates to the underling stream. However, in the case of `readBytesReference` and `readBytesRef` it leaves thw implementations to the standard `StreamInput` methods. These methods call the read byte array method which delegates to `ByteBufStreamInput`. The read byte array method on `ByteBufStreamInput` copies so it is safe. The only impact of this commit should be removing methods that could be dangerous if they were eventually called due to some refactoring.
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Oct 24, 2017
* master: Timed runnable should delegate to abstract runnable Expose adaptive replica selection stats in /_nodes/stats API Remove dangerous `ByteBufStreamInput` methods (elastic#27076) Blacklist Gradle 4.2 and above Remove duplicated test (elastic#27091) Update numbers to reflect 4-byte UTF-8-encoded characters (elastic#27083) test: avoid generating duplicate multiple fields (elastic#27080) Reduce the default number of cached queries. (elastic#26949) removed unused import ingest: date processor should not fail if timestamp is specified as json number
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
3 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.
This commit removes the
ByteBufStreamInputreadBytesReferenceandreadBytesRefmethods. These methods are zero-copy which means thatthey retain a reference to the underlying netty buffer. The problem is
that our
TcpTransportis not designed to handle zero-copy. The nettyimplementation sets the read index past the current message once it has
been deserialized, handled, and mostly likely dispatched to another
thread. This means that netty is free to release this buffer. So it is
unsafe to retain a reference to it without calling
retain. And wecannot call
retainbecause we are not currently designed to handlereference counting past the transport level.
This should not currently impact us as we wrap the
ByteBufStreamInputin
NamedWriteableAwareStreamInputin theTcpTransport. This streamessentially delegates to the underling stream. However, in the case of
readBytesReferenceandreadBytesRefit leaves thw implementationsto the standard
StreamInputmethods. These methods call the read bytearray method which delegates to
ByteBufStreamInput. The read bytearray method on
ByteBufStreamInputcopies so it is safe. The onlyimpact of this commit should be removing methods that could be dangerous
if they were eventually called due to some refactoring.