[8.x] Implement transactional events via marker interface

[mfn]

This is a PoC for fntneves/laravel-transactional-events#32 ; see below for the "official to be" Laravel PR description I intend to use and at the end, some notes regarding this.

---

What does it solve

A "classical problem" of systems with database transaction and events is that the latter is often handled void of the context of the current transaction.

This means in case of database transaction being rolled back, events are often already fired and thus their outcome might or might not reflect the state of the transaction anymore.

This PR attempts to provide a solution for that.

The implementation

Only events implementing the new TransactionalEvent interface are subject to this handling, the existing event behaviour is not changed (ensuring backwards compatibility).

This is based on https://github.com/fntneves/laravel-transactional-events with permission by the author Francisco Neves (see fntneves/laravel-transactional-events#32).

This is a MVP ^(*) implementation focusing on:

• providing the ability to internally store events
• discard them when a transaction is rolled back
• dispatch them when a transaction is commited
• supports nested transactions => events happening within nested transactions are handled on their own level, not affecting other transaction levels

These are slimmed down capabilities provided by the original approach because of doubts if automatic handling of e.g. Eloquent events for transaction is a good idea to start with or the additional complexity of pattern matching to perform on event names (please see the original readme).

Why only Events and not e.g. Jobs?

I often have to dispatch Jobs in transactions which I want to be "transactional safe"

First, why are "Jobs" also interesting to be handled in a transaction-safe way? Two examples:

• job for user registration email is fired, but for some reason the transaction was rolled back => yet the email was sent, now containing invalid information
  => this PR can help solving this
• jobs might get dispatched to external (or of course also internal) system, requiring access to the same database but are run outside the current transaction context.
  In such cases, it's not uncommon that these jobs see the wrong / outdated information in the database and thus might process stale data.
  There are ways to mitigate this, e.g. not passing model IDs but the whole model (Laravel supports serializing them), but this might not always be possible or desired
  => this PR can help solving this

Implementing this only for the event bus is a "basic building block". It allows to make transactional jobs with a bit of glue code: create a transactional job wrapper, tagged by the interface, and wrap the jobs you want to be transactional:

Add a \App\Events\TransactionalJob.php:

<?php declare(strict_types = 1);
namespace App\Events;

use Illuminate\Contracts\Events\TransactionalEvent;
use App\Jobs\Job;

class TransactionalJob implements TransactionalEvent
{
    private Job $job;

    public function __construct(Job $job)
    {
        $this->job = $job;
    }

    public function getJob(): Job
    {
        return $this->job;
    }
}

In the EventServiceProvider.php:

…
        TransactionalJob::class => [
            DispatchJob::class,
        ],
…

Add a \App\Listeners\DispatchJob.php:

<?php declare(strict_types = 1);
namespace App\Listeners;

use Illuminate\Contracts\Bus\Dispatcher;
use App\Events\TransactionalJob;

class DispatchJob
{
    private Dispatcher $dispatcher;

    public function __construct(Dispatcher $dispatcher)
    {
        $this->dispatcher = $dispatcher;
    }

    public function handle(TransactionalJob $event): void
    {
        $this->dispatcher->dispatch($event->getJob());
    }
}

and then use in your code like:

            $this->events->dispatch(new TransactionalJob(new MyActualJob($payload)));

(*) minimal viable product

Technical considerations:

• A new dependency to https://github.com/loophp/phptree is added; the existing library uses it for handling the nested levels of transactions correctly
• The package illuminate/event has to depend on illuminate/database to be able to register the event listeners for the transactions
  This is rather a hefty dependency for this, but I didn't see a way without it.
  Maybe future consideration: separate events to listen from individual packages and provide a way to separately depend only on them, much like illuminate/contracts works

Links

• laravel ideas issues 1441

---

Some notes

• There might be resistance to two changes necessary here:
  • Another new dependency => https://github.com/loophp/phptree (necessary for the nested transaction handling)
    Just a feeling…
  • Added dependency on https://github.com/illuminate/database (necessary for listening on the transaction events)
    Probably "fine" but some might not be happy about this; it's a hefty dependency but we need it makes the events for the transaction to listen on are in this namespace 😢
• I ported all the tests (#TIL : laravel/framework is only doing pure unit tests, so existing stuff like orchestra provides were out; it took my longer to adapt the tests then to port the actual code 🤪)
• I decided to use a trait because there are only two places of the existing code (constructor, dispatch method
• I tested this code "for real" in a private project (effectively replacing the fntneves/laravel-transactional-events library) and it worked (it also has a test suite with >7k tests with many making use of transactional event testing).

TODO

• Write more details within the commit messages; right now I kept them terse until the overall implementation is presentable. Especially regarding the original author
• Do we have enough coverage, should / can we do more?
  I checked the coverage and there are a few lines not covered it seems

[mfn]

Not sure if the big code example within the PR description could be misleading, as this kind of code isn't included (should be in a <details> tag maybe).

Also of what I remember, the majority of people longing for this actually want it for Eloquent native events (i.e. specifically the part not considered to be ported from the library), so we might add some useful arguments for that too.

[mfn]

"Native" Eloquent events can still benefit of this, similar to the Job example: they would just fire another event from within which is tagged with the interface.

[mfn]

Note: its for Laravel 8 because it requires laravel#32338 (and wouldn't have it for < 8)

[fntneves]

Hi @mfn,

I am really glad to see you're pushing it forward!
I'm a bit busy and will be for the next couple of weeks but I'm interested in working on this proposal.

At first glance, I see the dependency between events and database package will cause the most difficulties on merging this into the core. As it creates dependency between those two separated environments, I suspect Laravel folks will say that it should exist as a standalone package or at least be integrated in Laravel application rather than the core itself...

I like the approach of the TransactionalJob and it inspired me to add the possibility to specify transaction-aware custom instructions at runtime in a simple way. 👍

I've just subscribed to this topic, so I'm paying attention to this.

Thanks in advance!

[drupol]

I'm subscribing to the topic as well.
I'm the maintainer of loophp/phptree and I'm available if there is something to do in order to facilitate this PR.
Thanks!

[mfn]

I'm still alive and interested in moving forward 😄

• I rebased against current Laravel master and made sure it still works (it does ✅)
  • was good to do that because in the meantime \Illuminate\Support\Collection has been deprecated and I switched over to \Illuminate\Collections\Collection
• I updated the PRs intro description with a, what I think, is a better "sales pitch"

As a next step I plan to go through all issues/ideas in Laravel which every talked about such a feature and will ping people there to get a wider feedback.

Before doing that, any other feedback from current subscribers to this issue?

Thanks!

[mfn]

I've left a comment in issues/PRs I found which seemed relevant to invite people for more feedback:

• Eloquent events aware of database transactions. laravel/ideas#1441
• [5.7] Transactional jobs laravel/framework#26515
• Events are firing before committing the transaction laravel/framework#8627
• Race Condition: dispatch() saves uncommitted models to async queue laravel/framework#29710
• Scout indexes synced with stale data during transaction laravel/nova-issues#1906
• Transaction issue inside the eloquent observer saved event laravel/nova-issues#1759

[fntneves]

Hi @mfn,

Great to see it is ongoing and being discussed.

I read your PR introduction and I felt it is in the right way.
My suggestion is to add some known issues, like those you found and even more (I can grab other questions like this in Google/Reddit), to motivate the integration of this behavior in core.

[ilirien]

Hey guys!

Great job @mfn. A while ago I've faced some more issues with transactions and events.

I have similar implementation as @mfn provided. But when you don't need transactional events and you have some listeners that shouldn't be queued and some that should and you using SerialiazesModels it will throw exceptions ModelNotFound(if you creating it) or will return you incorrect data if job ran before transaction finish.

So here you have 2 solutions, do not queue this listeners or use delay(you can't be sure that transaction will be finished before delay is ended). All this solutions isn't perfect. So if you have ideas how to implement this behavior will be happy to hear.

Hope explained everything good, if you have questions feel free to ask.

[mfn]

@ilirien I don't fully understand

But when you don't need transactional events and you have some listeners

This PR/approach is about optionally enabling event only being dispatched when the database transaction successfully finishes; as such, the serialization part should work as expected.

Maybe you can try explain it again or, if you want, can you create a public repo showing the problem with this PR?

Thank you :-)

[ilirien]

@mfn

Not in all cases we need to delay running event in finished transactions, in some cases we need to delay specific listener to run after transaction finished.

My main message was that we need something like TransactionalListener for example. Some of the listeners should be ran immediatly some of them should be pended (for example that should be queued)

As well it will be great to have method similar to shouldQueue, because same events could be transactional in one case and not in other.

[fntneves]

@ilirien

Those seem very specific use cases, and you can create a specific event for what you call “delayed listeners”. The purpose of delaying events is to guarantee that the event represents what “did” happen and not what “possibly” happened.

That is, if a UserCreated event is raised, then the user was actually created an exists. As such, your listeners can handle that state change consistently. In my opinion, if you want to delay a listener, you should delay the event or even dispatch a delayed job inside a listener.

[mfn]

For all subscribers, it may be interesting to see laravel#35266

[mfn]

The above mentioned feature has made it into Laravel (and via some iterations it may seem to be solid), and docs are there: https://laravel.com/docs/8.x/queues#jobs-and-database-transactions

https://divinglaravel.com/better-management-of-database-transactions-in-laravel-8 mentions:

You can use the $afterCommit property on mailables, notifications, jobs, listeners, model observers, and broadcasted events.

My first impression is that it might obsolete this approach here, but I've not had time to verify. In case anyone else is interested to check this out and compare!

[hansnn]

@mfn

The Laravel changes might obsolete the implementation details of this PR (e.g. allow the implementation to be simplified), but not the feature itself.

As far as I have understood Laravel's new implementation, the only way to delay the dispatch of an Event is to wrap the dispatch itself in a DB::afterCommit closure. You would have to do that every time you dispatch an event inside a transaction if you want the event to represent what “did” happen and not what “possibly” happened (to quote @fntneves).

We ended up keeping fntneves/laravel-transactional-events because of this, so I'm still rooting for this PR. :)

[mfn]

Hmm, good point.

My first attempt in our a private codebase, just going for the jobs also didn't end up well. I faced the issue that jobs with ->afterCommit() and using the "fakes" in the tests, did still trigger dispatching the jobs when a transaction was rolled back: because it's the responsibility of the fake to respect this property (it seemed so to me; I wanted to make a quick-win change and failed, did not investigate further).

But yes, when I look around in the source tree of Laravel, I get the impression you've "only" two options: the manual DB::afterCommit() or the property on the jobs, but event itself are not covered:

I might be possible to emulate this though, this is a spontaneous idea I just had, did not verify:

• create a TransactionAwareEvent (not existing laravel-transactional-events)
• make it accept "any event" as payload
• make a listener for it which dispatches the event wrapped in a DB::afterCommit()

So I would "assume and think" that DB::afterCommit() is a low-level tool and it's possible to emulate the behavuor.

Though I'm not sure it properly supports nested transaction. Needs more testing/prototyping to find this out I guess.

[mfn]

Hey everyone 👋

I've decided to abandon to pursue this change becoming officially into Laravel.

The new features they added in the last couple months are not a replacement and don't cover the scope what is possible here: however I've doubts they would introduce a similar-yet-different approach on the whole thing, making effectively two competing strategies in Laravel.

As long as replacing the dispatcher works, it's "good enough" for me.

Thanks everyone for their thoughts and time!