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

Sliced ByteBuffer's Array Offset not respected #1662 #2144

Merged
merged 1 commit into from
Oct 22, 2018

Conversation

marc-christian-schulze
Copy link

@marc-christian-schulze marc-christian-schulze commented Sep 24, 2018

Fix for #1662

@plokhotnyuk
Copy link

Shouldn't proposed changes be synchronized with the following code for non-array based byte buffers?

        ByteBuffer copy = bbuf.asReadOnlyBuffer();
        if (copy.position() > 0) {
            copy.rewind();
        }

@marc-christian-schulze
Copy link
Author

rewind() only sets the buffer's position to zero - not the offset. So there is no need to change this.

@cowtowncoder cowtowncoder added 2.9 and removed 2.9 labels Sep 24, 2018
@marc-christian-schulze
Copy link
Author

Any chance to get this in 2.9.8? Would like to have this asap available.

@cowtowncoder
Copy link
Member

Right now I am swamped with work and have unfortunately little time to work on Jackson. But we'll see. I can't promise any specific date at this point, and it is unlikely 2.9.8 specifically would be released during October.

I am bit unsure if the change is safe enough to include in 2.9, however, and was considering it to go in 2.10.0 instead to reduce any risk of regression.

@marc-christian-schulze
Copy link
Author

marc-christian-schulze commented Oct 15, 2018

There are only two code paths leading to the change line:

  1. an array-backed ByteBuffer with offset of 0
  2. an array-backed ByteBuffer with offset > 0

For 1) there was already a unit tests and for 2) I created one that, if you remove my one line of change, will fail. So IMHO there's a negligible risk by publishing this change.

@cowtowncoder
Copy link
Member

@marc-christian-schulze Ok. ByteBuffer API has always been confusing to me so I will trust you understand it better and that this is legit. Merging to be included in 2.9.8. Thank you!

@cowtowncoder cowtowncoder merged commit f6ee23e into FasterXML:master Oct 22, 2018
@cowtowncoder
Copy link
Member

Would merge but Github is having... issues... it seems :-(

@cowtowncoder
Copy link
Member

Hmmh. Ok, didn't notice it's for master. Need to manually merge.

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

Successfully merging this pull request may close these issues.

3 participants