Remove DispatchConsumersAsync and add DispatchStrategies

[MichaelAlekseev]

Proposed Changes

This PR remove DispatchConsumersAsync property and add Dispatch strategies instead.

When I decided to use AsyncEventingBasicConsumer, I encountered an incomprehensible at that time error "Should never be called.". I was using version 6.0.0 of NuGet which didn't have the awesome "Enable 'DispatchConsumersAsync'." in exception message. After spending some time, I found that without DispatchConsumersAsync being set to true, the async consumer doesn't and won't work. This solution, in my opinion, has quite a few drawbacks. In particular:

1. If we use DispatchConsumersAsync = true we can create EventingBasicConsumer cause API allows. The error is not obvious.
2. If we use DispatchConsumersAsync = false we can create AsyncEventingBasicConsumer cause API allows. And again The error is not obvious
3. The solution, in my opinion, does not comply with the Liskov substitution principle
4. Use flag as behaviour indicator not obvious. I am sure that the record "Enable 'DispatchConsumersAsync'." appeared for a reason. It appearance indicates that something is wrong.
5. Single row difference - "await task.ConfigureAwait(false)" between AsyncConsumerDispatcher and ConsumerDispatcher again says that something is wrong.

Test RabbitMQ.Client.Unit.APIApproval.Approve failed cause API changed. Test RabbitMQ.Client.Unit.TestAsyncConsumer.TestBasicRoundtripConcurrentManyMessages fails in 10 seconds. However, no deadlock occurs. Locally, it takes me 22 seconds.

Types of Changes

What types of changes does your code introduce to this project? Put an x in the boxes that apply

• [ ] Bug fix (non-breaking change which fixes issue #NNNN)
• [ ] New feature (non-breaking change which adds functionality)
• [x] 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

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask on the mailing list. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

• [x] I have read the CONTRIBUTING.md document
• [x] I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• [ ] All tests pass locally with my changes
• [x] 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

Further Comments

The solution to the problem is rather trivial. Instead of creating a separate asynchronous Dispatcher types, we are starting to use a single Dispatcher. In order for us to be able to use a single Dispatcher for both types of Consumer (and maybe third and more), we need to use some kind of higher-level wrapper that would define behaviour. All of this is similar to the Strategy pattern. Using strategies instead of DispatchConsumersAsync allowed us to create a cleaner and more resilient architecture with a single IDispatchStrategy interface. If someday we need to add another strategy for another Consumer type, we will not need to create a 3rd Dispatcher. The wonderful Liskov substitution principle will help us out. In order to hide the implementation of artificial intelligence to determine which strategy needs to be created, a static DispatchStrategyFactory was created

[MichaelAlekseev]

@bollhals oh, thank you for your feedback and time! I will take your comments and try to find the more effective solution.

[MichaelAlekseev]

I think my bad that I was delete bool DispatchConsumersAsync at all. Maybe it should be marked as obsolete and test RabbitMQ.Client.Unit.APIApproval.Approve can pass then.

[bollhals]

I think my bad that I was delete bool DispatchConsumersAsync at all. Maybe it should be marked as obsolete and test RabbitMQ.Client.Unit.APIApproval.Approve can pass then.

That can be solved once the change itself is fine. Don't worry about this test. (Can be solved by updating the expected API, it's a textfile in the test AFAIR)

[luizcarlosfaria]

The main error is the client library doesn't not throw a exception when create a synchronous consumer, if connection of model is setted with DispatchConsumersAsync as true.

Today i found tests in source code about this.

Removing the DispatchConsumersAsync will produce behavior, configuration and public and common types impact.

It's dangerous! A change about this must have months of communication previously. To explain the change.

Today we can found 3.558 ocurrences of this in public repositories of github. From 2021 june to now, RabbitMQ.Client was downloaded ~9.2 milions of times.

This information must be in table before change things like that.

[lukebakken]

It's dangerous! A change about this must have months of communication previously. To explain the change.

@luizcarlosfaria this change will go into version 7, which is a major update with quite a few breaking API changes. They will be documented to the best of our ability.

If a user blindly upgrades from 6.x to 7 without taking the time to test, it's on them.

[lukebakken]

Closed due to lack of activity. I will make a note that version 7 should throw an exception if the correct dispatcher is not used.