Add ReadOnlyMemory overload to BasicPublish

[Anarh2404]

Proposed Changes

Add ReadOnlyMemory overload to BasicPublish.
That allows to send part of the array without creating a new one.

Types of Changes

• Bug fix (non-breaking change which fixes issue #NNNN)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause an observable behavior change in existing systems)
• Documentation improvements (corrections, new content, etc)
• Cosmetic change (whitespace, formatting, etc)

Checklist

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• All tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in related repositories

[michaelklishin]

I definitely like the idea but one thing in the implementation colored me surprised.

[stebet]

The byte[] overload for publish might be unnecessary now since byte[] can be implicitly cast to ReadOnlySpan. I'll review this later tonight :)

[bording]

The previous approach had a potential bug in that it would treat any IDictionary<string, T> as IDictionary<string, object>, so this is definitely better.

[bording]

The byte[] overload for publish might be unnecessary now since byte[] can be implicitly cast to ReadOnlySpan. I'll review this later tonight :)

@stebet I was just about to suggest the same thing. Seems like it makes sense to drop the byte[] overloads entirely.

[stebet]

Yeah. I'll review later and do some experiments.

[Anarh2404]

It makes sense. I can update this PR.
@michaelklishin @lukebakken what do you think?

[michaelklishin]

@Anarh2404 @stebet @bording there is a lot of code out there on the Internet that demonstrates using a byte array. Having an overload is a must but we can make it secondary to the one that uses ReadOnlyMemory. So, we always use ReadOnlyMemory internally but provide at least one additional overload.

We can update our tutorials use ReadOnlyMemory although we'd have to briefly explain what it is.

[bording]

@Anarh2404 @stebet @bording there is a lot of code out there on the Internet that demonstrates using a byte array. Having an overload is a must but we can make it secondary to the one that uses ReadOnlyMemory. So, we always use ReadOnlyMemory internally but provide at least one additional overload.

We can update our tutorials use ReadOnlyMemory although we'd have to briefly explain what it is.

The reason we're suggesting this is that nothing changes to the consumer. They continue to use byte arrays.

ReadOnlyMemory<byte> is a struct that has implicit conversions defined for byte[] and can also accept other backing sources.

There's no reason to keep having a byte[] overload when you have ReadOnlyMemory<byte>.

[michaelklishin]

OK, thanks for clarifying.

[stebet]

ReadOnlySpan is actually the lowest abstraction. If this can be modified to work with rhat, it should work with pretty much everything (Span, arraysegments, byte arrays, memory).

[Anarh2404]

ReadOnlySpan may be a problem when a fully async version is being developed, because ReadOnlySpan cannot be an argument of an async methods.

[michaelklishin]

Always great to both extend the API for more modern ways of doing things and remove the original API bits without losing backwards compatibility 😮 Thank you so much @Anarh2404 @bording @stebet! 👏