Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[BUG] There seems to be an atomicity issue while rising domain events in Ordering.Api #700

Closed
arielmoraes opened this issue Aug 8, 2018 · 6 comments

Comments

@arielmoraes
Copy link

arielmoraes commented Aug 8, 2018

In the Ordering service we have a ValidateOrAddBuyerAggregateWhenOrderStartedDomainEventHandler which handles the OrderStartedDomainEvent, after the Buyer is added or updated a call to SaveEntitiesAsync is made to dispatch the domain events and save the changes to the database, after that, a call to PublishThroughEventBusAsync is made. So, if the changes was already saved by calling the SaveEntitiesAsync method no atomicity will be in place or am I missing something? There is even a bigger problem if a chain of domain events are raised because only the last call to PublishThroughEventBusAsync will have atomicity between the current handled aggregate and the event.

@mvelosop
Copy link
Collaborator

mvelosop commented Aug 8, 2018

Hi @arielmoraes, that's a very good question! 👍

And the fact is that you're right, there's no global atomicity between microservices and that's actually on purpose!

Doing so would require the use of distributed transactions, using two phase commits, but that would cause coupling between the microservices, which is one of the key issues that we want to avoid in a microservices architecture, where each microservice has to be as independent as possible.

So, in a microservices architecture we are actually looking for eventual consistency by using an asynchronous message-based communication system, to propagate changes between microservices, in a pub/sub pattern.

And, just as you realized, doing so in a resilient way is very hard and requires more complexity than the one handled in eShopOnContainers right now.

Some ways to achieve the required resiliency for a production application would be using:

  1. The OutBox pattern
  2. Event sourcing or
  3. Transaction log minning

eShopOnContainers is just a sample to showcase architectural patterns and is not intended to be used in production as is, so the implementation of the Pub/Sub pattern is a basic one based on RabbitMQ.

You can get more details about this issue in @CESARDELATORRE's Microservices Architecture eBook on sections:

  • Asynchronous message-based communication (p. 57) and
  • Implementing event-based communication between microservices (integration events) (p.131)

Hope this helps.

@arielmoraes
Copy link
Author

Actually this is happening inside just one microservice, the Ordering microservice, which has two databases, one for storing the integration events and one for storing the aggregates. So the atomicity here is not between microservices but inside the Ordering microservice. I think the main idea is to save the aggregate only if the integration event is persisted, is that correct?

In this page
of the book there are two sections explaining the part related to the domain events:

  • The deferred approach for raising and dispatching events
  • Single transaction across aggregates versus eventual consistency across aggregates

So the problem is that there are two atomic operations happening here one is for the OrderingContext and the related domain events and one is for the OrderingContext and the IntegrationEventContext, but in fact all of them should be inside the same atomic operation, shouldn't them?

@mvelosop
Copy link
Collaborator

mvelosop commented Aug 9, 2018

Hi @arielmoraes,

It looks like I didn't get you right to begin with, will try to get it better this time.

Based on your input I traced calls, commands and events and seemed to get to a point where a DB commit is performed while dispatching a domain event cascading from another domain event, before the processing of first event finishes (the chained domain events you mentioned). But it got pretty complex at some point so will have to review it.

Anyway, it looks like atomicity is compromised here, but further investigation is necessary.

As for your previous comment, just a couple of clarifications:

  1. There's only one Ordering database that holds the tables for both the aggregates and the integration events logging.

  2. The strategy for handling domain events and integration events in eShopOnContainers should be (according to the documentation):

    1. Commit all changes in the microservice and insert the integration event log (atomically) as ready to publish in the one transaction and then

    2. Publish the integration event and mark the event as published in another transaction.

As the same document states, this strategy was chosen for the sake of simplicity, although it's known it doesn't handle all possible failure modes. (Take a look at page 139 from https://aka.ms/microservicesebook for a more recent content)

So, besides the "small" issue with atomicity you pointed out, it was a design decision to have two transactions, and leaving a trace for diagnosing. Although this is not probably adequate for a production application.

Another point is that there seems to be a issue with the implementation.

Hope this is a bit clearer now, and thanks a lot for your insights.

@mvelosop
Copy link
Collaborator

Marking this as a bug, there seems to be an important issue with transaction atomicity, it will be reviewed in depth.

@mvelosop mvelosop added the bug label Aug 22, 2018
@mvelosop mvelosop changed the title Is atomicity correctly implemented? [BUG] There seems to be an atomicity issue while rising domain events in Ordering.Api Aug 22, 2018
@mvelosop mvelosop reopened this Aug 28, 2018
@mvelosop
Copy link
Collaborator

This issue wasn't supposed to be closed, wonder who did it 🙄

@mvelosop
Copy link
Collaborator

mvelosop commented Feb 8, 2019

Hi @arielmoraes,

This was the chosen solution was, at high level:

  1. Begin a transaction in the TransactionBehaviour:

  2. While processing the related commands, add the resulting integration events to a list (an outbox):

    await _orderingIntegrationEventService.AddAndSaveEventAsync(orderStartedIntegrationEvent);

    There's a nice detail here, and it's that the outbox is persisted in the DB, so it'd be easy to implement a "watchdog" microservice that would handle possible failures in the publishing mechanism. This is not implemented in eShopOnContainers.

  3. Commit the transaction for all changes when returning from the behavior pipeline:

  4. Publish all integration events in the outbox:

    await _orderingIntegrationEventService.PublishEventsThroughEventBusAsync();

This strategy also handles the atomicity of all changes mentioned in issue #721

So I'm closing this issue now, but feel free to comment, will reopen if needed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants