Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure transaction safety #159

Closed
albe opened this issue Oct 20, 2017 · 5 comments
Closed

Ensure transaction safety #159

albe opened this issue Oct 20, 2017 · 5 comments
Assignees
Projects
Milestone

Comments

@albe
Copy link
Member

albe commented Oct 20, 2017

See #141 (comment)

Wrap ProcessManager lastAppliedSequenceNumber and state update into a single transaction

Without the two happening within a transaction, we potentially apply state transitions inside the PM multiple times.

Case:

event X comes in
PM updates state
crashes before updating lastAppliedSequenceNumber
-> catch up from the same event and reapply it after restart

@bwaidelich bwaidelich self-assigned this Feb 28, 2018
bwaidelich added a commit to bwaidelich/Neos.EventSourcing that referenced this issue Feb 28, 2018
This centralizes the invokation of `when*()` event handlers to a central
class that ensures the following invokation order:

# `beforeInvokingEventListenerMethod()` (when implementing `ActsBeforeInvokingEventListenerMethodsInterface`)
# `when*()` (the actual event handler)
# `saveHighestAppliedSequenceNumber()` (when implementing `AsynchronousEventListenerInterface`)
# `afterInvokingEventListenerMethod()` (when implementing `ActsAfterInvokingEventListenerMethodsInterface`)

This allows EventListeners to wrap the event handling in transactions for example:

```
class SomeEventListener implements AsynchronousEventListenerInterface, ActsBeforeInvokingEventListenerMethodsInterface, ActsAfterInvokingEventListenerMethodsInterface
{
  public function beforeInvokingEventListenerMethod(EventInterface $event): void
  {
    // start transaction
  }

  public function whenSomeEvent(SomeEvent $event): void
  {
     // db interaction
  }

  public function saveHighestAppliedSequenceNumber(int $sequenceNumber): void
  {
    // update sequence number in db
  }

  public function afterInvokingEventListenerMethod(EventInterface $event): void
  {
    // commit transaction
  }
}
```

This change...

* Renames `EventListenerLocator` to `EventListenerManager` – the class
  is now also responsible for *invoking* event listeners
* Removes `EventPublisher::catchUp()` and moves the remaining logic to
  the `EventCommandController`
* Introduces an `ActsAfterInvokingEventListenerMethodsInterface` marker
  interface, that is used similarily to `ActsBeforeInvokingEventListenerMethodsInterface`
  and allows to wrap event handling in a transaction
* Introduces a new `EventCantBeAppliedException` that allows for easier handling of
  exceptions during event handling (see neos#161)
* Removes an unused `projectionCache` configuration

Related: neos#159
@bwaidelich bwaidelich added this to the 1.0 milestone Mar 2, 2018
@bwaidelich bwaidelich added this to TODO in 2.0 via automation Mar 2, 2018
@bwaidelich bwaidelich changed the title Wrap ProcessManager lastAppliedSequenceNumber and state update into a single transaction Ensure transaction safety Mar 2, 2018
@bwaidelich
Copy link
Member

I renamed this ticket from "Wrap ProcessManager lastAppliedSequenceNumber and state update into a single transaction" in order to turn it into an umbrella ticket for the transaction-related issues:

EventListener last applied event & state updates

  • To implement a "safe" EventListener (not only ProcessManager) getHighestAppliedSequenceNumber(), when*(), saveHighestAppliedSequenceNumber() must be wrapped in a transaction (as mentioned above).
    Additionally there needs to be some kind of locking so that two processes can't interfere. SELECT ... FOR UPDATE is one option

Prevent multiple events with the same version

Luckily that is already guaranteed because of the unique key constraint in the events table. But we might want to wrap the corresponding exception (if two processes try to publish to the same stream "simultaneously") so that is easier to deal with – even though the risk of that is probably quite low

Exactly-once-delivery

We can't guarantee exactly once delivery (e.g. in ProcessManagers). We might be able to increase the chances that an event is processed exactly once using some kind of queue, but that is out of scope for this ticket

@bwaidelich bwaidelich moved this from TODO to In Progress in 2.0 Mar 2, 2018
@bwaidelich
Copy link
Member

bwaidelich commented Apr 6, 2018

I didn't get as far with this as I planned originally, but I spend some more thoughts on this:

The following activity diagrams are hard to read, but I can use them as memory aid for further discussions:

Note: HSN = Highest Sequence Number

Catchup all events per Handler in one transaction:

image
(http://yuml.me/edit/f7ce7c6f)

One transaction per event:

image
(http://yuml.me/edit/694f60cb)

Questions:

  • Which of the above is better?
  • Is it OK to "exit" if hsnNew != hsn + 1? (in the "One transaction per event" case)
  • How to ensure a transaction-safe HSN if none has been persisted yet? (INSERT ..., SELECT .. FOR UPDATE is not transaction-safe)
  • releaseHSN == rollBack()?
  • Which class is responsible for which task?
  • Handle catchup and replay differently?
  • Reverse responsibility of HSN? (in the Framework by default, EL can "opt in" via custom interface)
  • (controversial one): Move from pull to push via queue for event handling?

@albe
Copy link
Member Author

albe commented Apr 16, 2018

I'd like to dig into this topic again, and I tried twice already, but it's really deep and I'm a bit out of it. I hope to find some time for deep diving into this sometime in the near future, but for the moment, that won't be possible.

@bwaidelich
Copy link
Member

@albe Thanks for your comment! The list above was more of a mental aid in order to be able to discuss this with others. In the meantime I have a working(tm) solution that allows multiple workers to work in parallel without blocking / re-applying the same event twice. But it needs cleanup and a better architecture.. I tend to create a new branch so that I can push my intermediate state more often...

Would be cool to have your input from time to time, it is greatly appreciated!

bwaidelich added a commit to bwaidelich/Neos.EventSourcing that referenced this issue Sep 13, 2018
@bwaidelich
Copy link
Member

There are still some related issues (#245 for example), but the "AppliedEventsStorage" mechanism should be safe now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
2.0
  
Done
Development

No branches or pull requests

2 participants