Inline dispatcher

[JanEggers]

Proposed Changes

this pr adds a new dispatcher that reduces the runtime costs of dispatching.

Method	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated
Publish_Hello_World	457.7 ms	8.77 ms	19.79 ms	1.00	0.00	3000.0000	-	-	11.91 MB
Publish_Hello_World_Inline	420.6 ms	8.30 ms	16.18 ms	0.93	0.06	1000.0000	-	-	5.76 MB

I will add tests if the design is approved

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

[bollhals]

Why inherit from ConsumerWorkService?

[JanEggers]

because there is some fancy logic in modelbase ctor to create IConsumerDispatcher. Ideally this class should not need to exist. If I am allowed to refactor modelbase to just take IConsumerDispatcher as ctor parameter and place the dissision whats the implementation somewhere else im happy to do so. I just wanted to comply with the current patterns as close as possible.

[bollhals]

This is a valid point 👍 Didn't came accross this yet. Let's clean this up in another PR, so we don't pollute this one.

[bording]

Why did you decide to make this optional and expose a configuration switch?

From my quick pass through the PR, this appears to be an internal implementation change that would be safe to just do. If the new implementation is better, why keep the old one?

[JanEggers]

From my quick pass through the PR, this appears to be an internal implementation change that would be safe to just do. If the new implementation is better, why keep the old one?

becuase choices matter :) with the old implementation there was concurrent dispatching which might be a desired feature for some users.

my desire is to have the lowest overhead possible. If I need concurrent dispatching i can do it on the application layer.

If the maintainers of this package decide that the old implementation has no value then im fine to remove it but I did not want to enforce my opinion.

[JanEggers]

updated numbers after rebase

Method	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated
Publish_Hello_World	496.6 ms	9.76 ms	19.94 ms	1.00	0.00	2000.0000	-	-	10.81 MB
Publish_Hello_World_Inline	448.1 ms	8.75 ms	12.82 ms	0.91	0.05	1000.0000	-	-	4.74 MB

[bording]

becuase choices matter :) with the old implementation there was concurrent dispatching which might be a desired feature for some users.

Ah, I didn't catch that you lose concurrent dispatching with this approach. That's probably something that should be mentioned in the the original post for the PR! 😄

[danielmarbach]

@JanEggers I think it would be useful to add more context PR when the choices described here do matter in your context. Because without much more context one can only compare based on the benchmark. But the important question as always is if the additional choice that introduced (and thus the additional path to maintained) warrents the scenarios that are unlocked or not.

Maybe that could then also lead to a slightly better description of the current inline property to help guide the user of the flag to make a concious choise that makes sense for the scenario.

[JanEggers]

there are currently 2 dispatchers in the lib that work together with a worker service that influence the way messages are dispatched to the user.

default: ConcurrentConsumerDispatcher + ConsumerWorkService can change IConnectionFactoryConsumerDispatchConcurrency
this scheduler is active by default. with it you can use IBasicConsumer to consume messages in a sync method which does not allow async processing but still provides the capabilities to process multiple messages concurrently

+ concurrent scheduling
- no async support
- increased latency due to overhead in memory (work item) and threading

opt in: AsyncConsumerDispatcher + AsyncConsumerWorkService
to enable it set IConnectionFactory.DispatchConsumersAsync and IConnectionFactoryConsumerDispatchConcurrency to control concurrency
with this dispatcher you can use IAsyncBasicConsumer to consume messages in an async method (one that returns a task) and have multiple messages processed concurrently

+ concurrent scheduling
+ async support
- increased latency due to overhead in memory (work item) and threading

the new dispatcher introduced by this pr:
InlineConsumerDispatcher + InlineConsumerWorkService (useless but needed to maintain instantiation code)
to enable it set IConnectionFactory.DispatchConsumersInline. IConnectionFactoryConsumerDispatchConcurrency does not apply to this dispatcher
with it you can use IBasicConsumer to consume messages in a sync method which does not allow async processing and there is no buildin scheduling to process messages concurrently.

- no concurrent scheduling
- no async support
+ minimal latency

But the important question as always is if the additional choice that introduced (and thus the additional path to maintained) warrents the scenarios that are unlocked or not.

@danielmarbach can you please also elaborate why the current design is the way it is?

from my point of view there is still the option to just delete all the scheduling and the options on the connection factory in the impl and provide them soly through different implementations of IBasicConsumer. that way the user of this package could still decide what to use and is not depending on implementation bits in this library and he could mix sync and async so the overhead of the async scheduler only applies when you actually need it. imagine you have 20 subscriptions and just one has an async code path. right now you have to use the async scheduler and pay the overhead for all 20 subscriptions or create a second connection.

[danielmarbach]

@danielmarbach can you please also elaborate why the current design is the way it is?

@JanEggers I think this is probably a question to the maintainers of this lib. I joined as a community contributor around the v4 timeframe and already there the current design of having the relationship between the consumer and the work service was there. From the outside perspective, we contributed extensions to these existing concepts to allow to have async consumers. From the perspective of concurrently consuming messages, the library never offered much and forced the users of the libs to actually deal with these concepts. Over time the code required to support concurrent consuming gradually went down from

• Custom offloading to the worker thread pool (sync concurrent processing)
• Using tricks like async void to achieve fire & forget while allowing async dispatches of user code
• Needing to use special task schedulers like ConcurrentExclusiveSchedulerPair to avoid things interleaving
• Having the need to copy the payload on the synchronous path before yielding to avoid things being screwed up due to the buffer renting changes introduced

towards just handle things currently without having to worry about all the above

Just one example of the webinar I gave about the client evolution

From https://github.com/danielmarbach/AsyncRabbitMQ.Webinar/blob/master/Concurrency/Program.cs#L94-L138

to

https://github.com/danielmarbach/AsyncRabbitMQ.Webinar/blob/master/Concurrency62/Program.cs#L83-L93

I think this is a rather philosophical discussion about what a client should be. Some people prefer clients to be as "dump" as possible and argue all advanced stuff should be done by the users building stuff on top of it while others prefer clients to be as convenient as possible for end-users. Some others would even argue both things should be offered in the client for people to make a choice. As an example Azure Service Bus SDK propably falls into the last category because it allows you to have convenience like registering handlers and the concurrency is taking care off (including acking or nacking messages) while you can also fall back to manually receive and dispatch in ways that suits your needs.

For me this is another thing about this project that I wish would be made explicitely about the design choices of the owners of this project so that contributors can help to drive the lib in the right direction. I can see that all three approaches work and can found pros and cons for either of these. I only have one sample size of a webinar where 40 people attended and they expressed that they really love the new convenience concurrent consumption possibilities the lib now provides because not all people have the skills and knowledge to deal with concurrency.

I don't want to give the impression I'm shooting down your idea here! I'm not. So maybe the history of the project tells us that choices for users of the client are wanted and thus what you introduced is another choice for advanced scenarios where people really need to optimize for the last percentile (which I doubt most consumers of this lib fall into).

To the question of why the weird Consumer hierarchy is in place, I think it is a consequence of this project not having clear stewardship when it comes to taking larger directions (no offense please @michaelklishin @lukebakken I know you are doing the best you can while having to juggle other libs/clients etc) and thus contributors mostly tried to make things possible gradually by "hacking things into the lib" like we did in the past (and yes I'm also guilty of this). To me it seems once a certain path is taken the follow-up steps required to clean up things and consolidate design choices are not made visible and are suspected to the motivation or amount of time an individual contributor can or is willing to invest into this lib

This essay was written with the best intentions for all people involved

[stebet]

Very well said @danielmarbach. I agree that a clear design philosophy and roadmap is needed to better focus on where the library should head and what is really needed going forward, but that being said, I know @michaelklishin and @lukebakken have been doing a great job, but I also realize maintaining a popular OSS library is not easy either.

In my mind, there is also a lot of room for improvement in making it easier to extend and enhance the functionality of the library through clearer interfaces and abstractions. The library codebase is very much influenced by earlier .NET and Java designs, which is completely understandable given how old it is, but it's age is also starting to show, and improvements are needed i.m.o to make it more easily used in the modern .NET landscape.

I know everyone's intentions here are good and discussions around them is good as well, so I hope noone takes this personally. I'm writing this because I care :)

[JanEggers]

should i prepare another pr that moves all the scheduling logic into IBasicConsumer implementations to get a feeling how that would look like?

[JanEggers]

or is IObservable an option?

rabbitmq could expose an IObservable<?> and you would just need to link the rx.net docs on how to process them concurrently

[lukebakken]

@danielmarbach you and other community contributors have much more relevant .NET experience. As for the long-term direction this is my understanding of what the RMQ team would like to see in all official client libraries:

• A bare-bones, minimal API that allows users to do whatever they want. Basically, the AMQP protocol exposed.
• A "safe by default" API that hides some of the complexity of the above. Ideally it would prevent or have sensible defaults with regard to some of the most common mistakes when using RabbitMQ, such as using non-durable entities, non-persistent messages, and incorrect use of publisher confirmations. This API would still be simpler and lower level than what a project like EasyNetQ or NServiceBus provides.

My understanding is that version 7 of this library can be a clear break from the past.

[stebet]

@lukebakken that sounds like a good starting point indeed! Would it make sense to follow through with a call/meeting we were thinking earlier this year and establish some sort of guidelines and documented general direction?

[bollhals]

@danielmarbach you and other community contributors have much more relevant .NET experience. As for the long-term direction this is my understanding of what the RMQ team would like to see in all official client libraries:

• A bare-bones, minimal API that allows users to do whatever they want. Basically, the AMQP protocol exposed.
• A "safe by default" API that hides some of the complexity of the above. Ideally it would prevent or have sensible defaults with regard to some of the most common mistakes when using RabbitMQ, such as using non-durable entities, non-persistent messages, and incorrect use of publisher confirmations. This API would still be simpler and lower level than what a project like EasyNetQ or NServiceBus provides.

My understanding is that version 7 of this library can be a clear break from the past.

For me personally this should be the goal. A very nice example of this is the IBasicConsumer / DefaultBasicConsumer / EventingBasicConsumer (Yes granted this is very basic but still). I leaves you with freedom (interface) if you want to do it better suited for your purpose or you can use some default implementation provided by the library.

[lukebakken]

Would it make sense to follow through with a call/meeting we were thinking earlier this year and establish some sort of guidelines and documented general direction?

Yep, it does make sense however finding time will be a challenge.

[danielmarbach]

@lukebakken then I would recommend to have a v7 milestone and assign "issues" with goals like the ones you just described to it. Then subsequent work being done or needing to get done can be added to those "umbrella" issues and linked accordingly. With that discussions can happen on those high-level issues and once alignment is reached those issues will capture the main points in the description of the issue. If you add labels to the overarching topics you can easily use them as sort of "swimlanes" represent stream of work and make it visible

The dotnet team does something similar https://dotnetepics.azurewebsites.net/. It can be much more lightweight

[danielmarbach]

@bollhals

I think moving the dispatching into the model is a great idea because it also allows to move the important dispatch settings like concurrency into the consumer itself and then there is no longer the dualism of having to configure the connection factory to be async just to plug in the async consumer. I do agree to that.

@JanEggers Honestly I'm personally not a big fan of observables for libs like this. For the past number of years I have been trying to build async and concurrent APIs that are expressive by just using the built in .NET mechanisms like Task, ValueTask... and try to avoid observables because they really only are useful when you than take a dependency on Rx later.
But that is just my two cents.

[JanEggers]

I tried to move the dispatching logic into the consumer implementation for another PR but that gets recursive because the dispatcher wants to dispatch onto the same interface.

so this approach needs more thoughts