Feature Request: Allow to take over the payload in BasicDeliver call

[zgabi]

Since in version 6.x the data which is received by the consumer is returned to the buffer pool after the call is finished some users (like me) needs to copy that data.

Could you please add a functionality which allows the users to take over the data, so it is not returned to the pool? So the user can return it when it is not needed anymore. (By getting it from the ReadOnlyMemory<byte> with MemoryMarshall.TryGetArray, or any other way (*))

For example as in this commit: zgabi@11e72bd

(*): For example passing a custom struct instead of the ReadOnlyMemoryparameter, which has a Takeover or similar method.

[michaelklishin]

You can create a copy when needed, the docs have a dedicated section that mentions this. Without significantly more interest in this feature, we will leave things as they are.

[michaelklishin]

That said, I do like the idea you have in your fork @zgabi. But I would strongly prefer to not have "two modes of operation" where there's already a clear solution available: copy the memory in your consumer, and maybe even introduce a base consumer implementation if you need this in multiple places.

[zgabi]

Yes, I read in the documentation that it must be copied.. .but why do you force the users for a slow something when there could be a better solution?

The idea to take over the data is from rabbitmq client, it already exists in the internal code:
https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/b3e7a1470b43954003291d39930672b52f3cf743/projects/RabbitMQ.Client/client/impl/CommandAssembler.cs#L141-L1500

So internally you are also keeping the buffer when possible (probably for performance reasons), it would be nice to allow it for the users, too.

By the way, I think the return value in my fork is not too nice... It would be better to use a custom struct as I mentioned above, like:
zgabi@e9e9622

What do you mean about "two modes of operation"? This is not two modes, just a possibility to do something in the user code.
Of course this is breaking change, so not possible in the current version.

And the documentation can be changed for the next version, as it was changed for version 6.x.

[michaelklishin]

@stebet @bollhals do you have an opinion here? This would be a breaking API change but we can ship one in 7.0.

[bollhals]

This should be fairly simple to add for 7.0.
In order to understand the need though, would you mind sharing in which scenario you'd see this being used? (Complexity of public interface vs actual usage)
From my knowledge, people usually directly process it within the receiving method into the actual thing (usually either to a string or directly to an object). So in your case, why is this not the chosen way and is the potential performance gain really relevant?
(The internals pass it along, as you've analyzed correctly due to performance reasons (benefits are very limited though <2% AFAIR))

[zgabi]

In our application we are forwarding camera stream to the client applications. (Sometimes 4-5 HD stream / client). The video is processed with FFMpeg in a different thread. This is a live video (controllable cameras), so the delay should be as small as possible, so each video packet is sent in a different message queue item. (In addition a new requirement for us - which is under development - to forward VHF radio communication where the delay is even more important)

I know that it is not so optimal to put through video stream in a message queue, but it is a requirement from our customer to communicate only in a single port on a single IP address with a RabbitMQ. So this is why I would be happy to avoid all unnecessary copying. And the world goes to this direction. In dotnet core there are a lot of similar improvements with the new Span feature... RabbitMQ Client also has a lot of similar improvements since v 5.x. Everybody is trying to avoid unnecessary memory copying :)
So the current RabbitMQ Client implementation is acceptable, we are copying the data to a new rented buffer, but it would be even better to avoid that copy, too. Even if it is only 2%.

[bollhals]

This is surely an unusual usage of the client. So if we apply any changes, it should not have any negative effect on the existing hot path. How this could look like will have to be defined.
Whether or not to support this additional API will have to be decided first.

[stebet]

@zgabi Have you measured the cost of copying the data into your own memory pool (utilizing recycling there as well). That seems like it'd solve the problem without any client changes needed. Adding a public facing API to do this would expand the API with features that very very few people would ever take advantage of. I'm just thinking of the maintainability cost vs. demand here.

[zgabi]

@stebet I haven't measured the cost of the copy, since as I said, the current solution is acceptable for us.. There was a big (noticable) improvement between 5.x and 6.x versions, so it is ok now.

But the reason for this change is not only the possibility to get the data memory without copying that..
It has another advantage which makes the API cleaner... the parameter instead of the ReadOnlyMemory<byte> is another type which do not allow the users to create a reference to the data....

So with the current API, somebody can store the given ReadOnlyMemory<byte> data parameter to a field and use that value later after returning from the BasicPublish method... And that data will be invalid..
With this change it is not possible anymore, since the data is accessible only with the Span property,which cannot be stored in a field. It helps the users the avoid this type of problem. Calling the Span property of the custom struct will return empty span after returning from the BaiscPublish method. (Which I think is much better than receiving an invalid data)

[bollhals]

It has another advantage which makes the API cleaner... the parameter instead of the ReadOnlyMemory<byte> is another type which do not allow the users to create a reference to the data....

True, but changing from ReadOnlyMemory to ReadOnlySpan would bring the same benefit here.

[zgabi]

Yes, that is true...

but probaby your team choosed ReadOnlyMemory because EventintBasicConsumer needs to store the data in the event args.

rabbitmq-dotnet-client/projects/RabbitMQ.Client/client/events/EventingBasicConsumer.cs

Lines 87 to 93 in 2a709aa

	public override void HandleBasicDeliver(string consumerTag, ulong deliveryTag, bool redelivered, string exchange, string routingKey, IBasicProperties properties, ReadOnlyMemory<byte> body)
	{
	base.HandleBasicDeliver(consumerTag, deliveryTag, redelivered, exchange, routingKey, properties, body);
	Received?.Invoke(
	this,
	new BasicDeliverEventArgs(consumerTag, deliveryTag, redelivered, exchange, routingKey, properties, body));
	}

So there are 3 possibilities:

• remove this class
• use a custom struct, which does not allow (outside of RabbitMQ) to get the memory data.
• keep it as it is now.

[JanEggers]

I think @zgabi initial request is valid.

there should be an option to disable arraypool.return in the library and let the user decide when to return the array.

from by point of view there are two apis possibilies:

one is a sync api that provides the raw data as a ref struct whilch does not allow to store the data in the heap and therefore can return it after the method returns.

or

one that supports async and provides the data and lets the user decide when to retun it

@zgabi can you also have alook at #953 that improved msg latency by 10% (at least in my benchmark that uses a very small payload) so maybee that would be also benificial for you