Pick a single ByteBuffer implementation for all public APIs

[yschimke]

Currently java.nio.ByteBuffer is predominantly used for public APIs. Have a few issues, e.g. you really need to defensively shallow copy with bb.duplicate(). Also not always clear whether the intention is to use position/limit that caller provided, or treat it as a byte[] holder.

Some public APIs e.g. Frame expose Agrona and its used in metadata package. The netty packages use Netty's buffer, but mostly this is internal.

A single buffer API should be used which would make it easier to be sure that code is correct.

[stevegury]

IIRC the reason why we stick to java.nio.ByteBuffer has to do with the fact that we expose/leak the type via the Payload interface.
@tmontgomery @benjchristensen or @NiteshKant can you confirm?

[NiteshKant]

@stevegury Yes we need a ByteBuffer class to represent data and metadata Payloads and this issue couldn't have been better timed when I am fighting with potential bugs caused by using 3 different buffer types 😞

@yschimke you are not the only one who have issues with java.nio.ByteBuffer API 😉 and thanks for asking the question! Apart from the annoying flip(), rewind() methods and the lack of writerIndex there are complexities which aren't required from our usage point of view, eg: differentiation between direct and on-heap buffers.

I think the primary benefit of using java.nio.ByteBuffer is that it is a part of the JDK and hence a standard (good or bad is debatable). So, the question is whether a standard here is helping us or hurting us as we know that for internal and many external code, we have to downcast to native buffer APIs like agrona. Whatever, it is, it certainly isn't great that we have 3 buffer APIs to deal with, each having subtle differences that cause hard to debug bugs (you pointed to the capacity() and limit(), remaining differences between agrona and JDK buffer. From netty's buffer point of view, at some places we were using .nioBuffer() which is a view of the underlying buffer (pooled in some cases) and hence can change over time.)

I would like to propose that we stop using JDK buffer API and define a restricted, usable buffer API for ReactiveSocket

This may sound extreme to some but it will provide the following benefits:

• reactivesocket-core doesn't have to deal with two buffer abstractions (JDK and agrona) as it does today.
• Any buffer related optimization can reside in respective transport implementation. Aeron implementation can have its optimizations with agrona buffer and any implementation directly using that implementation will be aware of the optimization details and side-effect contracts. Netty can have its optimization related to pooling this new API buffer instance, instead of netty's buffers.
• There is a clear boundary between external buffer libraries and reactivesocket buffers.

Just to be clear, I don't believe we ever need to provide any implementation of our buffer API as we always read/write what is provided by the user/transport

I think there may be cases where we would trade some performance for clarity and maintainability of code but I think we should have strong data points that it indeed is the case. In absence of which, I would like to prefer clarity over micro-optimizations (assuming individual transports have done the optimizations required)

[yschimke]

This is pretty much what Finagle did https://github.com/twitter/util/blob/develop/util-core/src/main/scala/com/twitter/io/Buf.scala

Although personally, I'd be interested if we could survive with just Netty's buffers and the handover to agrona still be efficient.

[NiteshKant]

@yschimke we can chose netty's buffer as the library for ReactiveSocket but I think it is too broad an API for our use and has ref-counting as a core part of the API, which I am unsure of whether we need it or not.

[benjchristensen]

I'm not a fan of locking into to Netty as a dependency.

[benjchristensen]

To be clear, I really like Netty, but ReactiveSocket should not require someone using Netty. There are plenty of other options out there. For example, if using Aeron as the transport, someone shouldn't have to bring along Netty for the ride just for the Buffer type. And if someone is using one of the many alternatives to Netty for TCP, including just standard JDK libraries, again, they shouldn't have to bring along Netty.

Even Agrona is a dependency I'd prefer not having as I strongly prefer zero-dependency libraries whenever possible.

[NiteshKant]

@benjchristensen so looks like you are inclined towards defining our own buffer abstraction or you prefer using JDK buffers?

[yschimke]

I'll start on commons-buffers using runtime classloading tricks to choose an implementation at runtime :)

I kid... I kid...

[benjchristensen]

inclined towards defining our own buffer abstraction or you prefer using JDK buffers

Whichever works better. Our own buffer abstraction as an interface may work best as it avoids the concrete implementation of JDK.

Can we have the interface behave similar to DuplexConnection where we interact with just the interface, and then the binding implementation is provided by whatever provides DuplexConnection?

In other words, a ReactiveSocket can't be created without a concrete DuplexConnection, and without a concrete Buffer factory?

[NiteshKant]

Can we have the interface behave similar to DuplexConnection where we interact with just the interface, and then the binding implementation is provided by whatever provides DuplexConnection?

The first part is what my thinking was that we don't have to instantiate a buffer ourselves which removes the necessity of providing a factory. Although, users would have to provide Payload instances which would require a Buffer implementation. We can provide buffer instances created using a byte[] if a user does not have the buffer from a transport. This may just be "good enough".

[NiteshKant]

I'll start on commons-buffers using runtime classloading tricks to choose an implementation at runtime

@yschimke I think that is a good idea (#maniacalLaughter)

[robertroeser]

@yschimke
I'd prefer to just to use the nio ByteBuffer only if we want one library. It's standard and everyone can use it. We don't have to write anything to make it work with other libraries - it just does. The api isn't the greatest but it's standard so people know how to use it. The ReactiveSocket code is already based on ByteBuffer internally. Everyone supports it so it's not opinionated.

[robertroeser]

Also - we already chose ByteBuffer
#6

[yschimke]

This seems pretty good now

Agrona byte buffer usage seems limited to

• internal packages
• aeron transports
• a few factory methods e.g. Frame.from