`DefaultPayload.create(BytBuf data, ByteBuf metadata)` should release given BB after content copying

[OlegDokuka]

Actual

DefaultPayload.create(BytBuf data, ByteBuf metadata) leaves given ByteBufs unreleased which differs from the behavior of ByteBufPayload which ensures that given content is released when ByteBufPayload is released

Expected

DefaultPayload.create(BytBuf data, ByteBuf metadata) releases the given ByteBufs so a user should not take care of that.

The source of such expectation can be found there -> https://github.com/rsocket-routing/rsocket-routing-broker/blob/master/rsocket-routing-broker-spring/src/main/java/io/rsocket/routing/broker/spring/cluster/ClusterJoinListener.java#L158

Also, a Payload can be counted as the last handler in the chain of ByteBufs processing and should take care of its releasing

cc/ @spencergibb @rstoyanchev

[rstoyanchev]

As long as it's clearly documented what it does I think it can be either way. It's possible the caller may want to continue to use the buffers after create.

[OlegDokuka]

Well. I do not think mutating buffer makes any sense further after passing it into a codec. Do we have real example of such? If not, i would expect Payload to be the last one so it would take care of releasing the given.

[rstoyanchev]

Maybe not mutating, but for subsequent reading. Say I create a Payload in an auxiliary component (like an interceptor) but otherwise don't want to leave it alone for the main component to also consume. Or maybe I'm repeatedly using some buffers to create the same payload.

[OlegDokuka]

It is not consistent in any case since ByteBufPayload will do release it later. So now you write code like the following:

PayloadAllocator allocator = ... // here we do not know for sure whether it is ByteBufPayload or DefaultPayload;

ByteBuf data = ...;
ByteBuf metadata = ...;

Payload p = allocator.create(data, metadata);

// now We do release since we don't need that information anymore and we used to do so with DefaultPayload but now allocator allocates ByteBufPayload instead 

data.release();
metadata.release();

Following that abstract example, DefaultPayalod will work as expected, whilst ByteBufPayload will fail since

public static Payload create(ByteBuf data, @Nullable ByteBuf metadata) {
    ByteBufPayload payload = RECYCLER.get();
    payload.data = data;
    payload.metadata = metadata;
    // unsure data and metadata is set before refCnt change
    payload.setRefCnt(1);
    return payload;
  }

it just put given ByteBufs into internal fields.

That said, that implementations are inconsistent with each other. For future improvement of Payload API, it prevents creating consistent abstract API.

Coming to the comment:

Maybe not mutating, but for subsequent reading.

Usually, one does retain before calling send() if ByteBuf is needed for future reuse.

Thus I disagree that the current behavior of either DefaultPayload or ByteBufPayload is valid.

Moreover, the most common use-case is create and delegate rather than create and take care of it. The second just complicate things and will put more responsibility on the user to always do an extra release.

E.g.

DefaultPayaload.create(bb1, bb2)
bb1.release();
bb2.release();

vs.

DefaultPayload.create(bb1, bb2);

of course second is the best because this is the most common use-case.

The cases like:

1. Reuses
2. Read later again

is rather an exclusion than the common use

[rstoyanchev]

Okay for consistency it makes sense to align the behavior of ByteBufPayload and DefaultPayload so if generic code deals with either it can do the same.

[OlegDokuka]

fixed in #886