Remove dangerous `ByteBufStreamInput` methods (#27076)

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.
This commit is contained in:
Tim Brooks 2017-10-24 08:51:14 -06:00 committed by GitHub
parent d5efc30968
commit a7fa5d3335
1 changed files with 11 additions and 11 deletions

View File

@ -33,7 +33,6 @@ import java.io.IOException;
class ByteBufStreamInput extends StreamInput {
private final ByteBuf buffer;
private final int startIndex;
private final int endIndex;
ByteBufStreamInput(ByteBuf buffer, int length) {
@ -41,26 +40,27 @@ class ByteBufStreamInput extends StreamInput {
throw new IndexOutOfBoundsException();
}
this.buffer = buffer;
startIndex = buffer.readerIndex();
int startIndex = buffer.readerIndex();
endIndex = startIndex + length;
buffer.markReaderIndex();
}
@Override
public BytesReference readBytesReference(int length) throws IOException {
BytesReference ref = Netty4Utils.toBytesReference(buffer.slice(buffer.readerIndex(), length));
buffer.skipBytes(length);
return ref;
// NOTE: It is unsafe to share a reference of the internal structure, so we
// use the default implementation which will copy the bytes. It is unsafe because
// a netty ByteBuf might be pooled which requires a manual release to prevent
// memory leaks.
return super.readBytesReference(length);
}
@Override
public BytesRef readBytesRef(int length) throws IOException {
if (!buffer.hasArray()) {
return super.readBytesRef(length);
}
BytesRef bytesRef = new BytesRef(buffer.array(), buffer.arrayOffset() + buffer.readerIndex(), length);
buffer.skipBytes(length);
return bytesRef;
// NOTE: It is unsafe to share a reference of the internal structure, so we
// use the default implementation which will copy the bytes. It is unsafe because
// a netty ByteBuf might be pooled which requires a manual release to prevent
// memory leaks.
return super.readBytesRef(length);
}
@Override