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

DirectBuffer.wrap(ByteBuffer) does not respect wrapped buffer ordering #161

Closed
cowwoc opened this issue Jan 28, 2019 · 7 comments
Closed

Comments

@cowwoc
Copy link

cowwoc commented Jan 28, 2019

Following up on #40, please:

  1. Add Javadoc to the wrap() method (as opposed to a class-level comment) explaining that the input buffer is expected to have native ordering.
  2. Check the input buffer and if it does not have native ordering throw an exception.

I have wasted hours debugging problems as a result of this issue.

@cowwoc
Copy link
Author

cowwoc commented Jan 28, 2019

On second thought, the explanation provided for #40 doesn't make sense to me. At the time of this writing UnsafeBuffer.wrap() is not thread-safe so you might as well respect the order of the buffer passed in.

Further, sometimes we don't have the ability to mutate the input ByteBuffer. I have a use-case where a Java library writes into a ByteBuffer using BIG_ENDIAN. I then need to wrap this buffer in UnsafeBuffer but because you ignore the buffer ordering I am forced to manually flip the contents of the buffer. This isn't nice from a performance or usability perspective.

@mjpt777
Copy link
Contributor

mjpt777 commented Jan 28, 2019

If you look at all the method implementations the byte order of the wrapped buffer is never queried. All operations regarding byte order are stateless. All the accessor method have an overload for byte order with native being default without the additional arg.

Your performance point is not correct. I think you are misunderstanding the implementation.

@cowwoc
Copy link
Author

cowwoc commented Jan 28, 2019

@mjpt777 If I understand you correctly, you are saying that users should:

  1. Note the endianess of the ByteBuffer being wrapped.
  2. When writing into the DirectBuffer always pass that same endianness in. For example, we should be invoking DirectBuffer.putLong(index, value, byteOrder) where byteOrder corresponds to the ByteBuffer's ordering.

Is that correct?

@mjpt777
Copy link
Contributor

mjpt777 commented Jan 28, 2019

I'd say know the endianess for the codecs you wish to use in the context of their protocol, then pass ByteOrder as appropriate. Just reading the byte order is risky and what if the buffer passed is incorrect anyway? Basically know your protocol.

@mjpt777 mjpt777 closed this as completed Jan 28, 2019
@cowwoc
Copy link
Author

cowwoc commented Jan 28, 2019

@mjpt777 I mean no disrespect but I don't understand how you can say that UnsafeBuffer is stateless when it contains mutable fields. If you mean something differ it would help to reword this part of the documentation.

Also, I noticed you updated the documentation for DirectBuffer.wrap() but not UnsafeBuffer.wrap(). I think the latter might also need to be updated.

Regarding "know your protocol", makes sense.

@mjpt777
Copy link
Contributor

mjpt777 commented Jan 28, 2019

I've updated the Javadoc regarding "stateless". To be clear the wrap methods are the exception.

@mjpt777
Copy link
Contributor

mjpt777 commented Jan 28, 2019

To help clarify. It is not immutable because the fields are mutable. It is stateless in that there is no remembered state from one call to the next, e.g. your byte order example. The one exception is wrapping another buffer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants