Add DispatchConsumersAsync to IConnectionFactory

[paulomorgado]

If I receive an IConnectionFactory instance in my component, I have no way to know if it has DispatchConsumersAsync set to true or false.

public MyComponent(IConnectionFactory connectionFactory, string queueName)
{
    var connection = connectionFactory.CreateConnection();
    var model = connection.CreateModel();

    var consumer = new AsyncEventingBasicConsumer(this.model);
    consumer.Received += (s, e) => this.HandleRequestAsync(s, e);

    this.model.BasicConsume(
        queue: queueName,
        autoAck: true,
        consumerTag: string.Empty,
        noLocal: false,
        exclusive: false,
        arguments: null,
        consumer: consumer);
}

There's no way to know if this is going to work or not.

Assuming that it might be a bad idea to have a setter for DispatchConsumersAsync in IConnectionFactory, at least a getter would be nice.

public MyComponent(IConnectionFactory connectionFactory, string queueName)
{
    var connection = connectionFactory.CreateConnection();
    var model = connection.CreateModel();

    IBasicConsumer consumer:

    if (connectionFactory.DispatchConsumersAsync) // or connection.DispatchConsumersAsync
    {
        consumer = new AsyncEventingBasicConsumer(this.model);
        consumer.Received += this.HandleRequestAsync;
    }
    else
    {
        consumer = new EventingBasicConsumer(this.model);
        consumer.Received += (s, e) => this.HandleRequestAsync(s, e).Wait();
    }

    this.model.BasicConsume(
        queue: queueName,
        autoAck: true,
        consumerTag: string.Empty,
        noLocal: false,
        exclusive: false,
        arguments: null,
        consumer: consumer);
}

[michaelklishin]

@kjnilsson @bording @danielmarbach @adamralph if you agree that the idea is sound, I'd name the getter IConnectionFactory#UsesAsyncConsumerDispatcher.

[adamralph]

@michaelklishin I think there's a problem in the shape of this API. The single concern of "I want to consume sync or async" is spread across two disconnected places: the setting of that property on the connection factory, and the choice of consumer class, where we pass the model, which is itself at least two method calls away from creating the connection factory. Would it be possible remove the property altogether and encapsulate everything in required in each consumer class?

[michaelklishin]

We don't have such a property yet. Or am I misunderstanding your suggestion @adamralph? Moving this kind of context information to the consumer interface makes sense to me. I can see the argument that it's not really a consumer implementation detail per se but that's probably where the user would expect to see it.

[adamralph]

If I receive an IConnectionFactory instance in my component, I have no way to know if it has DispatchConsumersAsync set to true or false.
...
There's no way to know if this is going to work or not.

This implies that, to choose async or sync consumption, not only do you have to choose which consumer class to create, you also have to configure the connection factory correctly, which is used to create a connection, which is used to create a model, which is passed to the constructor of the consumer class. These two things are far apart. I'd prefer if I my choice of async or sync is expressed purely by which consumer class I choose to create.

[danielmarbach]

That would indeed be better. At the time when I wrote the implementation, it was meant to be a non-breaking and additive feature that adheres to the current design. If I recall correctly the factory owns the connections and the connections own the worker service which is responsible to dispatch stuff to the consumers. That's why the entry point of the connection factory was chosen to be the configuration point. It was also aligned with the multiple consumer models that were already there. I also tried to stick to the lifecycle of objects as much as possible. I seem to remember that the worker service is created before the consumers get assigned. Obviously, it is possible to do the same checks for the consumers and later create the worker service based on the consumer that is chosen.

[paulomorgado]

@danielmarbach, that's why I proposed to add a way to check the connection (and the model, since I'm asking) for asynchronous capabilities.

[paulomorgado]

Or, as some APIs (like file system) do, if the connection is not async, allow fake async by blocking on the consumer/model side.

[danielmarbach]

Got that. Was merely providing context

[paulomorgado]

Just found out that, if the connection is in async mode, using an EventingBasicConsumer will throw an InvalidOperationException with the message "In the async mode you have to use an async consumer".

Kind of a way to detect if it's in sync or async mode. 😄

[danielmarbach]

Hehe. It's software with a history thus like planes: nobody knows why they fly even when passengers have their cells not on flightmode🤣

[bording]

Needing to rely on an exception for control flow is certainly not ideal!

Overall I agree with @adamralph that it would be best if the choice of which consumer class was used was all that was needed to be async or not would be the best design here.

However, I do know that making that happen would require some substantial internal redesign, as @danielmarbach pointed out.

Based on that, it seems like exposing some additional property to at least let the value be discoverable when you need it would a decent way to move forward for now.

However, adding such a property to an interface would be a breaking change, so this should only be added in a new major version.

[paulomorgado]

Meanwhile?

[michaelklishin]

Currently "how async" a consumer is is determined in part by the connection factory settings used. I don't see this changing any time soon. Therefore introducing IConnectionFactory#UsesAsyncConsumerDispatcher sounds unavoidable to me for the time being.

[paulomorgado]

I think it should go down to as close as the consumer is created. It needs to be exposed in the IConnectionFactory, IConnection and IModel interfaces.

[stebet]

One question.. is there a reason consumers should NOT be async by design but provide synchronous methods as extras for those cases, thereby rendering this flag obsolete and unneeded.

Example:
Triggering consumers would always run the "Received" handlers asynchronously but the non-async consumers would simply provide synchronous methods that run the async handlers and return a Task.CompletedTask.