You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tuweni.apache.org by GitBox <gi...@apache.org> on 2020/12/18 07:50:34 UTC
[GitHub] [incubator-tuweni] Nashatyrev opened a new issue #88: Bytes.fromByteBuf() incorrectly interpret ByteBuf content
Nashatyrev opened a new issue #88:
URL: https://github.com/apache/incubator-tuweni/issues/88
### To reproduce
```java
ByteBuf buf = Unpooled.buffer(100).writeByte(1);
Bytes bytes = Bytes.wrapByteBuf(buf);
System.out.println(bytes.size());
System.out.println(bytes.toArray().length);
```
### Expected behavior
```
1
1
```
The `buf` content is only 1 byte, other 99 reserved bytes are not the part of the buffer content
### Observed behavior
```
100
100
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] Nashatyrev edited a comment on issue #88: Bytes.fromByteBuf() incorrectly interpret ByteBuf content
Posted by GitBox <gi...@apache.org>.
Nashatyrev edited a comment on issue #88:
URL: https://github.com/apache/incubator-tuweni/issues/88#issuecomment-657482389
> `Bytes.wrapByteBuf(buffer, 0, buffer.readableBytes())`
The more correct usage then would be: `Bytes.wrapByteBuf(buffer, buffer.readerIndex(), buffer.readableBytes())`
> I’d say the expected output is 100. A byte buffer is not variably sized, but rather sized at creation. Its content is 100 bytes. You just have 99 that are undefined.
I would tend to disagree here:
```java
ByteBuf buf = Unpooled.buffer().writeByte(1);
Bytes bytes = Bytes.wrapByteBuf(buf);
System.out.println(bytes.size());
```
This would output `256`. If you write `300` bytes, the output would be `512`. The buffer size is not static and managed internally. Another Netty version may potentially allocate other buffer size by default which makes `Bytes.wrapByteBuf()` behavior inconsistent.
Just another sample: that would output `1`
```java
ByteBuf buf = Unpooled.buffer().writeByte(1);
ByteBuf wBuf = Unpooled.wrappedBuffer(buf);
Bytes bytes = Bytes.wrapByteBuf(wBuf);
System.out.println(bytes.size());
```
The 'true' `ByteBuf` content starts at `readerIndex` and has size `readableBytes`. While other buffer content is still available via `ByteBuf` API it should be treated as junk. When allocating buf via `Unpooled.directBuffer()` you may even potentially have uninitialized memory in the rest of the buffer.
I see the option of `Bytes.wrapByteBuf(ByteBuf, int, int)` but I still think the method `Bytes.wrapByteBuf(ByteBuf)` default behavior should be to wrap only the readable content of `ByteBuf`.
I would reopen the issue for the next round of discussions
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] Nashatyrev commented on issue #88: Bytes.fromByteBuf() incorrectly interpret ByteBuf content
Posted by GitBox <gi...@apache.org>.
Nashatyrev commented on issue #88:
URL: https://github.com/apache/incubator-tuweni/issues/88#issuecomment-747296527
> If you think the existing behavior is the one which is expected (or may be already relied on by other libs), I would add a Javadoc comment and and an alternative method which wraps the ByteBuf readable contents only.
Hey, I would still suggest to address this issue in one or another way. It's definitely not a showstopper, but I'm pretty sure there would definitely be misreading and erroneous usage of this method in the future.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] Nashatyrev commented on issue #88: Bytes.fromByteBuf() incorrectly interpret ByteBuf content
Posted by GitBox <gi...@apache.org>.
Nashatyrev commented on issue #88:
URL: https://github.com/apache/incubator-tuweni/issues/88#issuecomment-657482389
> `Bytes.wrapByteBuf(buffer, 0, buffer.readableBytes())`
The more correct usage then would be: `Bytes.wrapByteBuf(buffer, buffer.readerIndex(), buffer.readableBytes())`
> I’d say the expected output is 100. A byte buffer is not variably sized, but rather sized at creation. Its content is 100 bytes. You just have 99 that are undefined.
I would tend to disagree here:
```java
ByteBuf buf = Unpooled.buffer().writeByte(1);
Bytes bytes = Bytes.wrapByteBuf(buf);
System.out.println(bytes.size());
```
This would output `256`. If you write `300` bytes, the output would be `512`. Another Netty version may potentially allocate other buffer size by default which makes `Bytes.wrapByteBuf()` behavior inconsistent.
Just another sample: that would output `1`
```java
ByteBuf buf = Unpooled.buffer().writeByte(1);
ByteBuf wBuf = Unpooled.wrappedBuffer(buf);
Bytes bytes = Bytes.wrapByteBuf(wBuf);
System.out.println(bytes.size());
```
The 'true' `ByteBuf` content starts at `readerIndex` and has size `readableBytes`. While other buffer content is still available via `ByteBuf` API it should be treated as junk. When allocating buf via `Unpooled.directBuffer()` you may even potentially have uninitialized memory in the rest of the buffer.
I see the option of `Bytes.wrapByteBuf(ByteBuf, int, int)` but I still think the method `Bytes.wrapByteBuf(ByteBuf)` default behavior should be to wrap only the readable content of `ByteBuf`.
I would reopen the issue for the next round of discussions
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] atoulme commented on issue #88: Bytes.fromByteBuf() incorrectly interpret ByteBuf content
Posted by GitBox <gi...@apache.org>.
atoulme commented on issue #88:
URL: https://github.com/apache/incubator-tuweni/issues/88#issuecomment-657348338
Yeah, I tend to agree. I mean, we could create another Bytes implementation that goes by readable bytes. I think that'd be surprising though.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] Nashatyrev commented on issue #88: Bytes.fromByteBuf() incorrectly interpret ByteBuf content
Posted by GitBox <gi...@apache.org>.
Nashatyrev commented on issue #88:
URL: https://github.com/apache/incubator-tuweni/issues/88#issuecomment-658412055
> These are not features of the Bytes implementation in Tuweni
And this is exactly the reason why I would expect from `Bytes` to wrap only readable `ByteBuf` content
> Given Netty provides methods to present just the "readable" bytes as a ByteBuf, e.g. `Unpooled.wrappedBuffer(byteBuf)`
`Unpooled.wrappedBuffer(byteBuf)` is actually was just an example. It is normally used to 'concat' several `ButeBuf`s. And it concats only readable bytes.
It is absolutely regular use case for Netty to receive a wire frame in a `ByteBuf` e.g. `| header | payload |` and then a lower-level decoder reads the header and then passes the same `ByteBuf` to a higher-level decoder with a `readerIndex` positioned to the beginning of `| payload |` and the higher-level decoder treat that `ByteBuf` exactly as just only `| payload |`.
So in that case I would expect `Bytes.wrapByteBuf()` to return only payload bytes.
You may also check Google results on ["netty bytebuf to array"](https://www.google.com/search?client=firefox-b-d&q=netty+bytebuf+to+array)
If you think the existing behavior is the one which is expected (or may be already relied on by other libs), I would add a Javadoc comment and and an alternative method which wraps the `ByteBuf` readable contents only.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] cleishm commented on issue #88: Bytes.fromByteBuf() incorrectly interpret ByteBuf content
Posted by GitBox <gi...@apache.org>.
cleishm commented on issue #88:
URL: https://github.com/apache/incubator-tuweni/issues/88#issuecomment-657687195
What you're dealing with here is the semantics of Netty's `ByteBuf`. It contains pointers for readable and writable pointers and provides a method `Unpooled.buffer()` that provides a buffer who's capacity may vary between implementations, and it doesn't guarantee bytes are zeroed out. These are not features of the `Bytes` implementation in Tuweni.
Given Netty provides methods to present just the "readable" bytes as a `ByteBuf`, e.g. `Unpooled.wrappedBuffer(byteBuf)`, I'm not sure the implementation of `Bytes.wrapByteBuffer(...)` is definitively wrong or needs to be changed, as you have the option to achieve what you're looking for via `Bytes.wrapByteBuffer(Unpooled.wrappedBuffer(...))`. That said, I'm sure @atoulme would also happily consider an alternative constructor method that inlines this behavior.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] atoulme commented on issue #88: Bytes.fromByteBuf() incorrectly interpret ByteBuf content
Posted by GitBox <gi...@apache.org>.
atoulme commented on issue #88:
URL: https://github.com/apache/incubator-tuweni/issues/88#issuecomment-657277039
So I see that you go by readable bytes, and the implementation goes by capacity.
What's a good fix here? A new utility method that copies bytes out to a different buffer (as it seems like you run into issues with keeping a reference to the original ByteBuf anyway) or a fix to the class to go by readableBytes() as size?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] cleishm edited a comment on issue #88: Bytes.fromByteBuf() incorrectly interpret ByteBuf content
Posted by GitBox <gi...@apache.org>.
cleishm edited a comment on issue #88:
URL: https://github.com/apache/incubator-tuweni/issues/88#issuecomment-657337134
I’d say the expected output is 100. A byte buffer is not variably sized, but rather sized at creation. It’s content is 100 bytes. You just have 99 that are undefined.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] atoulme commented on issue #88: Bytes.fromByteBuf() incorrectly interpret ByteBuf content
Posted by GitBox <gi...@apache.org>.
atoulme commented on issue #88:
URL: https://github.com/apache/incubator-tuweni/issues/88#issuecomment-657355464
You can also do this:
`Bytes.wrapByteBuf(buffer, 0, buffer.readableBytes())` which will give you the right size.
Closing this issue, please feel free to reopen if there's more to this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] Nashatyrev edited a comment on issue #88: Bytes.fromByteBuf() incorrectly interpret ByteBuf content
Posted by GitBox <gi...@apache.org>.
Nashatyrev edited a comment on issue #88:
URL: https://github.com/apache/incubator-tuweni/issues/88#issuecomment-658412055
> These are not features of the Bytes implementation in Tuweni
And this is exactly the reason why I would expect from `Bytes` to wrap only readable `ByteBuf` content
> Given Netty provides methods to present just the "readable" bytes as a ByteBuf, e.g. `Unpooled.wrappedBuffer(byteBuf)`
`Unpooled.wrappedBuffer(byteBuf)` is actually was just an example. It is normally used to 'concat' several `ButeBuf`s. And it concats only readable bytes.
It is absolutely regular use case for Netty to receive a wire frame in a `ByteBuf` e.g. `| header | payload |` and then a lower-level decoder reads the `| header |` and then passes the same `ByteBuf` to a higher-level decoder with a `readerIndex` positioned to the beginning of `| payload |` and the higher-level decoder treat that `ByteBuf` exactly as just only `| payload |`.
So in that case I would expect `Bytes.wrapByteBuf()` to return only payload bytes.
You may also check Google results on ["netty bytebuf to array"](https://www.google.com/search?client=firefox-b-d&q=netty+bytebuf+to+array)
If you think the existing behavior is the one which is expected (or may be already relied on by other libs), I would add a Javadoc comment and and an alternative method which wraps the `ByteBuf` readable contents only.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] cleishm edited a comment on issue #88: Bytes.fromByteBuf() incorrectly interpret ByteBuf content
Posted by GitBox <gi...@apache.org>.
cleishm edited a comment on issue #88:
URL: https://github.com/apache/incubator-tuweni/issues/88#issuecomment-657337134
I’d say the expected output is 100. A byte buffer is not variably sized, but rather sized at creation. Its content is 100 bytes. You just have 99 that are undefined.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] cleishm edited a comment on issue #88: Bytes.fromByteBuf() incorrectly interpret ByteBuf content
Posted by GitBox <gi...@apache.org>.
cleishm edited a comment on issue #88:
URL: https://github.com/apache/incubator-tuweni/issues/88#issuecomment-657687195
What you're dealing with here is the semantics of Netty's `ByteBuf`. It contains pointers for readable and writable pointers and provides a method `Unpooled.buffer()` that provides a buffer who's capacity may vary between implementations and doesn't guarantee bytes are zeroed out. These are not features of the `Bytes` implementation in Tuweni.
Given Netty provides methods to present just the "readable" bytes as a `ByteBuf`, e.g. `Unpooled.wrappedBuffer(byteBuf)`, I'm not sure the implementation of `Bytes.wrapByteBuffer(...)` is definitively wrong or needs to be changed, as you have the option to achieve what you're looking for via `Bytes.wrapByteBuffer(Unpooled.wrappedBuffer(...))`. That said, I'm sure @atoulme would also happily consider an alternative constructor method that inlines this behavior.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] cleishm commented on issue #88: Bytes.fromByteBuf() incorrectly interpret ByteBuf content
Posted by GitBox <gi...@apache.org>.
cleishm commented on issue #88:
URL: https://github.com/apache/incubator-tuweni/issues/88#issuecomment-657337134
I’d say the expected output is 100. A byte buffer is not variably sized, but rather sized at creation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org
[GitHub] [incubator-tuweni] atoulme closed issue #88: Bytes.fromByteBuf() incorrectly interpret ByteBuf content
Posted by GitBox <gi...@apache.org>.
atoulme closed issue #88:
URL: https://github.com/apache/incubator-tuweni/issues/88
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tuweni.apache.org
For additional commands, e-mail: dev-help@tuweni.apache.org