[BUG] EventBusRabbitMQ Message Processing Problem

[falamarzijahromi]

I'm examining RabbitMqEventBus functionality with some integration and functional tests for using it in a project. I found that according to RabbitMq.Client source code on the consumer, Received Event is an EventHandler<>, therefor the client library didn't handle for any asynchronous operations (Like Exception handling in async methods!).

///<summary>Event fired on HandleBasicDeliver.</summary>
public event EventHandler<BasicDeliverEventArgs> Received;

https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/master/projects/client/RabbitMQ.Client/src/client/events/EventingBasicConsumer.cs

But in EventBusRabbitMq, message consuming handled with an async subscription.

It will causes two problems:

1. Processing messages non-sequentially:
   As EventBusRabbitMq uses async subscription for the Received Event, It will causes to dedicate a separate Thread for every message processing (Threads count will be limited to the client QoS config). In this case for example if messages M1, M2 were in the queue, both of them will be processed simultaneously. In the cases that choreography of events were important (Like when events related to a specific entity) this will causes a disaster.
   Beside everything, queued messages must really be processed sequentially. Receiving every types of messages in one queue for a microservice (That this kind of design is in question itself!) won't be the reason to process messages non-sequentially.

2. Exception Handling problems:
   As RabbitMq.Client doesn't handle the Received Event asynchronously, any Exception throw during processing a message won't be caught, therefor causes application or test down. The subscribtion for CallbackException Event in the EventBusRabbitMqBus, won't be invoked in this case.

[MarcoNicolodi]

In the company i work on we've put a try/catch blog with logging for solving the second issue. At least the application is not being shut down.

[falamarzijahromi]

In the company i work on we've put a try/catch blog with logging for solving the second issue. At least the application is not being shut down.

Yep that works, but it'll make the consuming subscription to do two things, consuming and exception handling. if we want to be strict, we have to separate those two tasks for a cleaner code.

BTW, are you guys happy after applied this sample for the project as a platform? was it ok?

[mvelosop]

Hi @falamarzijahromi thanks for the heads up!

Problem #1 certainly looks like a bug, although as far as I could check, this doesn't have any real impact in eShopOnContainer, and that's why it actually hasn't bubbled up.

As far as I could check, all integration events that could "compete" somehow for the same Order, where the event ordering is important, are generated by Ordering.BackgoundTasks with a long enough cadence so it's highly improbable that the problem arises.

Anyway, I think it's something that has to be fixed, because it's inherently flawed, for the reasons you expose.

I found this post about the issue: http://gigi.nullneuron.net/gigilabs/asynchronous-rabbitmq-consumers-in-net/, and it should be solved by using an AsyncEventingBasicConsumer instead of EventingBasicConsumer

So the fix shouldn't be a problem now.

As for problem #2, I'm supposing that the AsyncEventingBasicConsumer should handle exceptions since it (should) handle the Task from the handler. (https://stackoverflow.com/questions/31961261/rabbitmq-asynchronous-support)

Thoughts?