8.0 Proposal: Deprecate synchronous consumers/dispatchers in favor of asynchronous consumers/dispatchers

[stebet]

I'd like to propose that the synchronous consumers/dispatchers be deprecated and removed from the 7.0 client in favor of asynchronous ones, to simplify the basic API.

Reasoning: Due to the asynchronous nature of message consumers, I think it doesn't make sense that consumers should have a synchronous interface. If the interface for consumers is asynchronous, they can still run synchronously if they do not require asynchrony by not utilizing the await keyword and returning cached tasks that are already completed.

Simplified examples:

// We could even remove the dispatcher interface altogether since it's internal anyways, but putting it here for clarity
internal interface IConsumerDispatcher
{
    bool IsShutdown { get; }
    ValueTask OnBasicConsumeOkAsync(IBasicConsumer consumer, string consumerTag);
    ValueTask OnBasicDeliverAsync(IBasicConsumer consumer, string consumerTag, ulong deliveryTag, bool redelivered, string exchange, string routingKey, IBasicProperties basicProperties, ReadOnlyMemory<byte> body, byte[] rentedArray);
    ValueTask OnBasicCancelOkAsync(IBasicConsumer consumer, string consumerTag);
    ValueTask OnBasicCancelAsync(IBasicConsumer consumer, string consumerTag);
    ValueTask OnModelShutdownAsync(IBasicConsumer consumer, ShutdownEventArgs reason);
    ValueTask Quiesce();
    ValueTask Shutdown(IModel model);
}

public interface IBasicConsumer
{
    IModel Model { get; }
    ValueTask OnConsumerCancelledAsync(ConsumerEventArgs reason);
    ValueTask OnBasicCancelAsync(string consumerTag);
    ValueTask OnBasicCancelOkAsync(string consumerTag);
    ValueTask OnBasicConsumeOkAsync(string consumerTag);
    ValueTask OnBasicDeliverAsync(string consumerTag, ulong deliveryTag, bool redelivered, string exchange, string routingKey, IBasicProperties properties, ReadOnlyMemory<byte> body);

    // Would this even be needed anymore? Couldn't consumers access and subscribe to this through their IModel instance anyways?
    ValueTask OnModelShutdownAsync(object model, ShutdownEventArgs reason);
}

If someone would want to provide a synchronous dispatcher, the implementer would simply not use await, call whatever synchronous methods should be called and return default; which in the case of ValueTask is a task that is simply completed. If the main client simply checks if the dispatcher method ran synchronously before awaiting, they automatically become synchronous and no task is required.

This is in-line with how .NET Framework/Core uses pluggable handlers in HttpClient.

[bollhals]

I like the proposal!
Two things I'd like to have a discussion about:

• OnBasicDeliverAsync has 7 parameters and is performance critical. Would it be beneficial to wrap them in a struct and pass by reference?
• ValueTask returns. I'm not the expert in this topic, but what benefits do we get by choosing the nongeneric valuetask over task for an API that mostly synchronously completed?

Regadring the sync version, isn't it discouraged to call .GetAwaiter().GetResult()? Link

[stebet]

I like the proposal!
Two things I'd like to have a discussion about:

• OnBasicDeliverAsync has 7 parameters and is performance critical. Would it be beneficial to wrap them in a struct and pass by reference?
• ValueTask returns. I'm not the expert in this topic, but what benefits do we get by choosing the nongeneric valuetask over task for an API that mostly synchronously completed?

Regadring the sync version, isn't it discouraged to call .GetAwaiter().GetResult()? Link

ValueTask is a struct and allocates nothing in the synchronous case. No need to call GetAwaiter etc, since it has a .IsCompletedSuccessfully property so awaiting can be skipped.

It's explained well here: https://devblogs.microsoft.com/dotnet/understanding-the-whys-whats-and-whens-of-valuetask/

[bollhals]

Yes, but Task doesn't allocate either in the sync case where nothing is returned. (Calls Task.CompletedTask)
ValueTask contains multiple fields and is thus bigger than a reference, which makes it more expensive to return.

I completely get the need of ValueTask in the sync completion case, but I somehow struggle to understand the ValueTask vs Task (non generic) benefit.

[stebet]

To quote the article: "With the advent of enabling even asynchronous completions to be allocation-free, however, a non-generic ValueTask becomes relevant again. Thus, in .NET Core 2.1 we also introduced the non-generic ValueTask and IValueTaskSource. These provide direct counterparts to the generic versions, usable in similar ways, just with a void result."

[bollhals]

Is my assumption correct that this is only beneficial if the non generic ValueTask is backed by a IValueTaskSource? Otherwise it's still allocating either way (Task & ValueTask)

[stebet]

Is my assumption correct that this is only beneficial if the non generic ValueTask is backed by a IValueTaskSource? Otherwise it's still allocating either way (Task & ValueTask)

Sort of, you can reuse ValueTasks and pool them.

[michaelklishin]

Moving to 8.0 so that we can ship a 7.0 with a smaller set of breaking API changes if necessary.