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

Fix DefaultDataBuffer#getNativeBuffer() to set correct limit #32009

Conversation

injae-kim
Copy link
Contributor

Closes gh-30967

Motivation

// DefaultDataBuffer
	public ByteBuffer getNativeBuffer() {
		this.byteBuffer.position(this.readPosition);
		this.byteBuffer.limit(readableByteCount()); // 👈 limit should be writePosition, not readableByteCount
		return this.byteBuffer;
	}
  • We found that DefaultDataBuffer#getNativeBuffer set byteBuffer#limit as readableByteCount() incorrectly so fix to writePosition

Modification

  • Fix DefaultDataBuffer#getNativeBuffer() to set correct limit as writePosition instead of readableByteCount()

Result

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 10, 2024
* @return the wrapped byte buffer
*/
public ByteBuffer getNativeBuffer() {
this.byteBuffer.position(this.readPosition);
this.byteBuffer.limit(readableByteCount());
this.byteBuffer.limit(this.writePosition);
Copy link
Contributor Author

@injae-kim injae-kim Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0  1  2  3  4  5  6  7  8  9
                     ^        ^
                   read     write

When readPosition: 7 and writePosition: 10,
Current getNativebuffer return bytebuffer with limit: 3 (readableByteCount(), writePos - readPos)

But I think we should set limit: 10 (writePos) correctly.

Like this commit, maybe we set redableByteCount() as length on bytebuffer's limit by mistake 😅

@injae-kim injae-kim force-pushed the fix-defaultDataBuffer-getNativeBuffer branch from 177104b to cd85736 Compare January 10, 2024 17:38
Comment on lines +37 to +38
@Test // gh-30967
void getNativeBuffer() {
Copy link
Contributor Author

@injae-kim injae-kim Jan 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

FYI) with current getNativeBuffer() logic, this test failed~!

@jhoeller jhoeller added the in: core Issues in core modules (aop, beans, core, context, expression) label Feb 5, 2024
@poutsma poutsma added this to the 6.2.0-M1 milestone Feb 7, 2024
@jhoeller jhoeller added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Feb 7, 2024
@poutsma poutsma closed this in 70004e9 Feb 20, 2024
poutsma added a commit that referenced this pull request Feb 20, 2024
…iveBuffer

* gh-32009:
  Polishing external contribution
  Set correct limit in DefaultDataBuffer::getNativeBuffer
@poutsma
Copy link
Contributor

poutsma commented Feb 20, 2024

Thanks for supplying a PR. I decided to change the fix by returning a duplicate with independent position and limit, instead changing these on the buffer itself. This is arguably the way it should have been since 5fc1806.

@injae-kim
Copy link
Contributor Author

injae-kim commented Feb 22, 2024

Oh thank you for your additional polishing :)

I'm so happy that this is my first contribution(969d0bd) on spring-framework!
I'll continue to contribute by another PR 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The method getNativeBuffer() in DefaultDataBuffer returns misconfigured ByteBuffer
4 participants