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

DataBuffer should offer restricted access to underlying ByteBuffer #29943

Closed
poutsma opened this issue Feb 7, 2023 · 0 comments
Closed

DataBuffer should offer restricted access to underlying ByteBuffer #29943

poutsma opened this issue Feb 7, 2023 · 0 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone

Comments

@poutsma
Copy link
Contributor

poutsma commented Feb 7, 2023

Netty 4's ByteBuf allowed for direct, unrestricted access of the underlying ByteBuffer(s) through the nioBuffers method. This allowed for integration with ByteBuffer-based APIs (including java.nio), but also opened the door for memory leaks. Spring 5 exposed nioBuffer through DataBuffer::asByteBuffer.

Netty 5's Buffer does not allow unrestricted access to the underlying ByteBuffer. As a consequence, in Spring 6 DataBuffer::toByteBuffer was introduced. This method makes a copy, and is implemented in Netty5DataBuffer::toByteBuffer using Buffer::copyInto.

In various places throughout Spring Framework 6, including frequently invoked code such as codecs and transports, DataBuffer::toByteBuffer is used to integrate with ByteBuffer-based APIs. This means that buffers are copied more frequently than in Spring 5, even in other server environments than Netty. See #29889.


Netty 5's Buffer does allow restricted access to the underlying ByteBuffer, through Buffer::forEachComponent. Memory leaks are negated by only allowing access to the raw memory for the scope of the returned iterator, like so:

  try (var iteration = buffer.forEachReadable()) {
          for (var c = iteration.first(); c != null; c = c.next()) {
              ByteBuffer componentBuffer = c.readableBuffer();
              // ...
          }
      }

We should expose Netty 5's Buffer::forEachComponent through a method in DataBuffer, implement that method in Netty 4 using ByteBuf::nioBuffers, and write an implementations for DefaultDataBuffer.

@poutsma poutsma added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement labels Feb 7, 2023
@poutsma poutsma added this to the 6.0.5 milestone Feb 7, 2023
@poutsma poutsma self-assigned this Feb 7, 2023
@poutsma poutsma changed the title Expose Netty 5's Buffer::forEachComponent DataBuffer should offer restricted access to underlying ByteBuffer Feb 8, 2023
mdeinum pushed a commit to mdeinum/spring-framework that referenced this issue Jun 29, 2023
This commit introduces DataBuffer::readableByteBuffers and
DataBuffer::writableByteBuffers, allowing restricted access to the
ByteBuffer used internally by DataBuffer implementations.

Closes spring-projectsgh-29943
mdeinum pushed a commit to mdeinum/spring-framework that referenced this issue Jun 29, 2023
mdeinum pushed a commit to mdeinum/spring-framework that referenced this issue Jun 29, 2023
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: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

1 participant