Eshoponcontainers: [BUG] Losing messages in Azure Service Bus subscription

Created on 3 Feb 2018  路  8Comments  路  Source: dotnet-architecture/eShopOnContainers

Hi.
I've realized there is a case when messages published into Azure Service Bus can be never handled.
This is the scenario:

  • The service (for instance Basket) is down.
  • Some events of different types are published into the service bs.
  • Basket service gets started.
  • When creating the subscription, there is a condition to only process messages if there is already a subscription saved in memory. But the message is still set as completed
  • This means that other type events are directly marked as completed even when they haven't been processed.

It can be solved just checking out the message has been actually processed and mark it as completed only in that case.
Please, let me know if you want me to make a pull request with it or it is something you already knew and you want to let as it is.

bug

Most helpful comment

@paulvanbladel - Yeah I've noticed this as well.

In the startup class we ask the service provider to return us an IEventBus which returns either a EventBusRabbitMQ or an EventBusServiceBus. In the constructor of those classes it creates a subscription and starts listening for messages on the topic.

var eventBus = app.ApplicationServices.GetRequiredService<IEventBus>();

eventBus.Subscribe<ProductPriceChangedIntegrationEvent, ProductPriceChangedIntegrationEventHandler>();
eventBus.Subscribe<OrderStartedIntegrationEvent, OrderStartedIntegrationEventHandler>();

But at that point we haven't added any subscribers! The EventBusServiceBus actually uses the subscribers in order to add rules to the subscription. So essentially we are processing events which our service might not handle or events which we do handle but haven't associated subscribers for.

For the 'EventBusServiceBus' you could remove the RegisterSubscriptionClientMessageHandler call from the constructor and manually call this after you've set subscribers...

var eventBus = app.ApplicationServices.GetRequiredService<IEventBus>();

eventBus.Subscribe<ProductPriceChangedIntegrationEvent, ProductPriceChangedIntegrationEventHandler>();
eventBus.Subscribe<OrderStartedIntegrationEvent, OrderStartedIntegrationEventHandler>();

eventBus.RegisterSubscriptionClientMessageHandler()

This would require making RegisterSubscriptionClientMessageHandler externally accessible.

For the EventBusRabbitMQ you could remove the CreateConsumerChannel from the constructor and call that after you've set subscribers.

All 8 comments

Yes please, submit a PR with the fix and explain the issue in the PR and the functional tests you did after fixing it, ok?
Thanks a lot! 馃憤

Hola C茅sar.
Sorry for my late answer on this.

The issue happens when you are subscribed to several kind of events.
For instance, in Basket.API you have two of them:
eventBus.Subscribe();
eventBus.Subscribe();

If an event is received when the service is up everything is ok.
But if the service is down and some messages are published, when the service starts up they can be lost.

That happens because inside Subscribe, pending events are processed but only if there is already a subscription for it.
So, in this example, when subscribing to ProductPriceChangedIntegrationEvent, if there is any OrderStartedIntegrationEventHandler it will be lost, as there is no subscription to it yet, but it is marked as completed anyway.

To solve it, I've added a single line where the event is marked as completed only if actually completed.
I've tried to make a pull request but I get an error 403 when pushing my changes to a fix branch. So, please let me know how I should process.

Are you trying to push to branch of the main repo?

Most of us do not have write privileges to the main repository.

I fork of the repository, I then make a branch in the forked repository and push changes to the branch in the forked repository. I then do the PR from the branch in the forked repository.

Hi. Sorry for my late answer.
Thanks for that, I just did it.

Cheers!

I'm experiencing similar problem in the rabbitMq version.
Is the problem not that during ProcessEvent the Subscriptionmanager is already used before it is actual initialized?
I'm looking at the dev branch and when putting breakpoints during startup, you can see that the whole rabbitms machinery is already ready before subscriptionmanager registered the events is should listen for.
In that scenario the the events simply got lost.

@paulvanbladel - Yeah I've noticed this as well.

In the startup class we ask the service provider to return us an IEventBus which returns either a EventBusRabbitMQ or an EventBusServiceBus. In the constructor of those classes it creates a subscription and starts listening for messages on the topic.

var eventBus = app.ApplicationServices.GetRequiredService<IEventBus>();

eventBus.Subscribe<ProductPriceChangedIntegrationEvent, ProductPriceChangedIntegrationEventHandler>();
eventBus.Subscribe<OrderStartedIntegrationEvent, OrderStartedIntegrationEventHandler>();

But at that point we haven't added any subscribers! The EventBusServiceBus actually uses the subscribers in order to add rules to the subscription. So essentially we are processing events which our service might not handle or events which we do handle but haven't associated subscribers for.

For the 'EventBusServiceBus' you could remove the RegisterSubscriptionClientMessageHandler call from the constructor and manually call this after you've set subscribers...

var eventBus = app.ApplicationServices.GetRequiredService<IEventBus>();

eventBus.Subscribe<ProductPriceChangedIntegrationEvent, ProductPriceChangedIntegrationEventHandler>();
eventBus.Subscribe<OrderStartedIntegrationEvent, OrderStartedIntegrationEventHandler>();

eventBus.RegisterSubscriptionClientMessageHandler()

This would require making RegisterSubscriptionClientMessageHandler externally accessible.

For the EventBusRabbitMQ you could remove the CreateConsumerChannel from the constructor and call that after you've set subscribers.

@heymega thanks a lot. Very useful !

Closing this issue now as the original issue was tackled with PR #582.

There was a new, related, issue appended here, but it'll be handled in issue #612.

Was this page helpful?
0 / 5 - 0 ratings