Eloquent events aware of database transactions.

[TitasGailius]

I has a situation where a database transaction fails but an eloquent event is still being dispatched.

To illustrate this problem take this test into consideration:

    /** @test */
    public function it_fires_an_eloquent_event_for_committed_transactions()
    {
        Event::fake();

        DB::transaction(function () {
            factory(User::class)->create();
        });

        Event::assertDispatched("eloquent.created: App\User");
    }

Everything works as expected. The user gets stored into the database and an event is fired.

Although if we manually fail the transaction, an event would still be fired:

    /** @test */
    public function it_fires_an_eloquent_event_for_committed_transactions()
    {
        Event::fake();

        DB::beginTransaction();
        factory(User::class)->create();
        DB::rollBack();

        Event::assertDispatched("eloquent.created: App\User");
    }

Problem:

The user is not stored into the database but an event is still being fired.

This creates situations where a specific action would be done even if they shouldn't. "Welcome email" is a perfect example, it would try to send the email even if the user is not there.

Suggestion:

Collect pending eloquent events and dispatch them only after the transaction is committed.

[TitasGailius]

Also in this example, an event is dispatched even if a user is not stored yet.

    /** @test */
    public function it_fires_an_eloquent_event_for_committed_transactions()
    {
        Event::fake();

        DB::transaction(function () {
            factory(User::class)->create();

            sleep(10);
        });

        Event::assertDispatched("eloquent.created: App\User");
    }

[mfn]

This generally a problem with all the events/listeners/dispatchers: nothing is aware of Transactions. I haven't come around an ActiveRecord system yet which doens't have this inherent problem.

My solution so far:

• built an abstraction around the transaction which covers the database and an in-memory queue of "collected events"
• every event I dispatch is first collected in aforementioned in-memory queue
• once the database transaction successfully finishes, all the jobs collected in-memory are dispatched

The "abstraction" I mentioned is a custom solution I built for that purpose: https://github.com/mfn/php-transaction-manager
You nest the transactions, the last/inner transaction is supposed to be Laravels database transaction and as it unravels the "success" (commit) phase, it first commits the inner database transaction and then the others which dispatches the collected jobs "for real".

Ruby has a similar open issue: rails/rails#26045

The also talk and consider another use-case which isn't "un-important": handling of such events when the Queue system is database based.

I personally don't use database-based queuing systems but they definitely have their use-case especially together with transactions.

TL;DR: innocent looking but complex topic 😄

[TitasGailius]

This generally a problem with all the events/listeners/dispatchers.

Well, I disagree about custom events because you can put these outside of a transaction manually, while eloquent is a different story. Also, there might be some events that you want to dispatch even if a transaction is not committed.

As a simple solution I think deferring only eloquent events would be fine.

[mfn]

As a simple solution I think deferring only eloquent events would be fine.

I guess it's context dependent (not disagreeing though).

I think the "perfect" solution would be an issue way to let the developer choose if and what events should honor transactions or not. Then again, maybe this is already the solution I had to implement: custom.

[deleugpn]

This is a classic problem from Message Driven systems. I had a few discussions about this at my workplace and the older coworker mentioned that this problem is at least 25 years old.

We pinned it down to a very subtle difference on the message sent to the Bus / Topic: A request for change vs A change happened. Pub/Sub systems rely heavily on the 2nd concept; when a message is published, it means something genuinely happened and the world has to deal with that however they see fit. The systems interested in that change will subscribe to the Message / Topic.
The 1st concept differs in the sense that a commitment hasn't been reached yet. In this case, you're being hostage of the 1st approach. You're putting a Listener / Subscriber in a Topic that only represents a Request For Change, not a genuine change. If you can rollback the user creation, then you should not listen to eloquent.created. You should dispatch your own custom event as soon as you commit. Only you know if the commit will actually happen or not.
By creating your custom event and dispatching it only after the commit takes effect, then your Topic / Event will actually carry the meaning of "A change has happened, do whatever you need to do about it".
The difference between these two are very subtle and lead to a lot of problems down the road because it's really easy to misuse a Topic / Event as an effective change when in fact it can simply be a request for a change.
One of the reasons why this is a delicate subject it's because it comes down to semantics. The semantic of a topic may change overtime without a clear reason.
Imagine you had a system that created a user and sent a welcome email without transactions. The eloquent topic / event was a genuine "change has happened". 3 months later your product requirements change and now the user creation is split into two steps and require a database transaction. At this moment, automatically, the semantic of the topic changed without any clear notice. As soon as you rollback because the 2nd step failed, the event dispatched no longer means "a change has happened". It instead changed to "A request for change" and the subscribers now either need to be rollbackable or move on to a secondary topic / event.

[matt-allan]

As a simple solution I think deferring only eloquent events would be fine.

The problem with this approach is sometimes you want the event to be handled inside of the transaction, and sometimes you don't.

For example, I might want to add an entry to the permissions_audit_log table every time someone changes something in the user_permissions table. I would want that listener to receive the event inside of the transaction, because I want to guarantee the audit log entry is written if the permissions write succeeds. If the event was received outside of the transaction there is no guarantee the audit log write would succeed.

On the other hand, if I am using an event to kick off a job I don't want to dispatch the job until the transaction commits. Otherwise the worker could run before the data it needs to use is visible.

I've sometimes seen these two types of events called 'domain events' and 'integration events'.

The way we solve this at work is by using a marker interface (Similar to ShouldBroadcast in Laravel) to denote events that are meant for other processes. If the event matches that interface and we are in a transaction, we delay firing it and queue it up in memory. Once the transaction commits we fire all of the queued events. If the transaction rolls back we discard all of the queued events.

It's possible to write an event subscriber to do this. You don't need to change how the Dispatcher works at all. Eloquent already emits transaction began, committed, and rolled back events that you can hook into. For example:

class TransactionHoldingSubscriber
{
    /**
     * @var bool
     */
    private $inTransaction = false;

    /**
     * @var array
     */
    private $queued = [];

    public function subscribe(Dispatcher $events): void
    {
        $events->listen(TransactionBeginning::class, function () {
            $this->inTransaction = true;
        });

        $events->listen(TransactionCommitted::class, function () use ($events) {
            $this->inTransaction = false;

            while ([$event, $payload] = array_pop($this->queued)) {
                $events->dispatch($event, $payload);
            }
        });

        $events->listen(TransactionRolledBack::class, function () {
            $this->inTransaction = false;
            $this->queued        = [];
        });

        $events->listen('*', function (string $event, array $payload) {
            if ($this->inTransaction) {
                $this->queued[] = [$event, $payload];
                return false;
            }
        });
    }
}

You can easily expand that example to only hold events matching an interface or pattern, i.e. if ($this->inTransaction && $event instanceof .... This also works well for queuing jobs.

This could be implemented as a package if someone feels inspired 😁

Note that there is a small risk the process might crash between the DB transaction committing and the events being fired. If that is unacceptable for you you can write events/jobs to the database first.

[mvanduijker]

Maybe we can take a look how Rails does it: https://guides.rubyonrails.org/active_record_callbacks.html#transaction-callbacks

[deleugpn]

Laravel already support Database events by emitting Illuminate\Database\Events\TransactionBeginning, Illuminate\Database\Events\TransactionCommitted and Illuminate\Database\Events\TransactionRolledBack, but that is not what this topic is about.

[mfn]

Also they are different from what Rails transaction callback does: Rails keeps track of the models mutated in the transaction and fires callbacks on a model basis.

Laravel only has the generic callback, void of any models.

[mvanduijker]

Just a brain dump by using a trait:

Trait:

trait TransactionalAwareEvents
{
    protected static $queuedTransactionalEvents;

    public static function bootTransactionalAwareEvents()
    {
        $events = static::getEventDispatcher();

        static::created(function (Model $model) {
            if (!DB::transactionLevel()) {
                return;
            }
            self::$queuedTransactionalEvents['created'][] = $model;
        });

        static::saved(function (Model $model) {
            if (!DB::transactionLevel()) {
                return;
            }
            self::$queuedTransactionalEvents['saved'][] = $model;
        });

        static::updated(function (Model $model) {
            if (!DB::transactionLevel()) {
                return;
            }
            self::$queuedTransactionalEvents['updated'][] = $model;
        });

        static::deleted(function (Model $model) {
            if (!DB::transactionLevel()) {
                return;
            }
            self::$queuedTransactionalEvents['deleted'][] = $model;
        });

        $events->listen(TransactionCommitted::class, function () {
            foreach (self::$queuedTransactionalEvents as $eventName => $models) {
                foreach ($models as $model) {
                    $model->fireModelEvent('after_commit_' . $eventName);
                }
            }
            self::$queuedTransactionalEvents = [];
        });

        $events->listen(TransactionRolledBack::class, function () {
            foreach (self::$queuedTransactionalEvents as $eventName => $models) {
                foreach ($models as $model) {
                    $model->fireModelEvent('after_rollback_' . $eventName);
                }
            }
            self::$queuedTransactionalEvents = [];
        });
    }

    public static function afterCommitCreated($callback)
    {
        static::registerModelEvent('after_commit_created', $callback);
    }

    public static function afterCommitSaved($callback)
    {
        static::registerModelEvent('after_commit_saved', $callback);
    }

    public static function afterCommitUpdated($callback)
    {
        static::registerModelEvent('after_commit_updated', $callback);
    }

    public static function afterCommitDeleted($callback)
    {
        static::registerModelEvent('after_commit_deleted', $callback);
    }

    public static function afterRollbackCreated($callback)
    {
        static::registerModelEvent('after_rollback_created', $callback);
    }

    public static function afterRollbackSaved($callback)
    {
        static::registerModelEvent('after_rollback_saved', $callback);
    }

    public static function afterRollbackUpdated($callback)
    {
        static::registerModelEvent('after_rollback_updated', $callback);
    }

    public static function afterRollbackDeleted($callback)
    {
        static::registerModelEvent('after_rollback_deleted', $callback);
    }
}

Model:

class MyModel extends Model
{
    use TransactionalAwareEvents;

    public static function boot()
    {
        parent::boot();

        static::afterRollbackCreated(function ($model) {
            dump("Created MyModel::{$model->id} is rolled back");
        });
    }
}

Tinker:

\DB::beginTransaction();
MyModel::create();
\DB::rollback();

[TitasGailius]

The problem with this solution is that event listeners defined in your trait must always be handled first. I solved my case by simply overriding fireModelEvent model method. It collects all model events and dispatches them only when the transaction is completed. It worked perfectly for my case and the solution is not too magical.

[mvanduijker]

@TitasGailius I understand your case. In this case we add extra events "after_commit_created", "after_commit_updated", etc and keep the working of the regular events in tact (they are performed within the transaction). You get the flexibility to act on events within the transaction (ie "created") and act on events after the transaction is committed (ie "after_commit_created") or rolled back (ie "after_rollback_created". Not that much different as in Rails. As far as I can see the ordering of the listeners doesn't matter. We just record what happened with the model and fire the correct event after the transaction is committed or rolled back.

[mvanduijker]

I created a package for transactional events https://github.com/mvanduijker/laravel-transactional-model-events

This will provide a trait you can put on your models and will fire extra events after commit or rollback. I am going to test it in one of my projects. If there is anyone willing to review / test it, I would really appreciate it.

[thejacer87]

@TitasGailius can you share your solution?

[mfn]

There was a package mentioned for this in #1441 (comment)

However I think it falls short because it requires to adapt your models and your code for new events.

I believe there might be a better suited package: https://github.com/fntneves/laravel-transactional-events (not mine)

It works transparently (if wanted) and, within transactions, prevents events from being immediately fired but does so afterwards. The readme is quite insightful so I'm not going to replicate it here.

But my TL;DR is: if Laravel could out of the box provide some "protection" about this problem, that would be real Killer Feature for applications built with it requiring this.

[lindyhopchris]

Personally I don't think that any change is required, as this problem is entirely avoided by having a clear distinction between the use of Eloquent events and custom (business logic) events.

We use the post-change Eloquent events (e.g. created, saved etc) solely for database changes that need to be made as a result... i.e. they occur within the transaction.

All business logic events then fire after the transaction. E.g. if you're creating a new user, the Registered event fires post database commit, not during the transaction. I.e. because the user has only registered if they are in the database.

Eloquent events aren't massively useful for business logic, because you never know why the model has been stored/changed. E.g. you don't need to send a welcome email to a user being created in the database via a seeder, but you do when they register via the UI/API. That context is important.

TLDR: use Eloquent events only for database stuff you want to happen within the transaction, use business logic events outside of the transaction.

[TitasGailius]

@lindyhopchris Imagine using Laravel Scout. You want to automatically update the index whenever a model is updated. If the update is happening inside a transaction that fails, it would still update the index even tho the actual record hasn't changed.

Another example. If you have a slug attribute on your model and you want to make sure that whenever title attribute changes it updates the slug attribute as well. Change is happening inside a transaction that fails, it still updates the slug attribute.

I believe there are many cases where the correct behaviour would solve this kind of issues.

[lindyhopchris]

Imagine using Laravel Scout. You want to automatically update the index whenever a model is updated. If the update is happening inside a transaction that fails, it would still update the index even tho the actual record hasn't changed.

Don't queue the listener. Then it happens within the transaction.

Another example. If you have a slug attribute on your model and you want to make sure that whenever title attribute changes it updates the slug attribute as well. Change is happening inside a transaction that fails, it still updates the slug attribute.

We'd using saving()/updating() to do that.

To go back to the original example:

    /** @test */
    public function it_fires_an_eloquent_event_for_committed_transactions()
    {
        Event::fake();

        DB::beginTransaction();
        factory(User::class)->create();
        // non-queued listeners execute here, i.e. any database changes they make
        // occur within the transaction and get rolled back.
        DB::rollBack();

        Event::assertDispatched("eloquent.created: App\User");
    }

Really the original test isn't logically correct, it should be:

    /** @test */
    public function it_fires_an_eloquent_event_for_committed_transactions()
    {
        Event::fake();

        DB::beginTransaction();
        factory(User::class)->create();
        Event::assertDispatched("eloquent.created: App\User");
        DB::rollBack();
    }

So as an example, if you were creating the user via a registration service that might rollback the user it creates, the following test would be logically correct:

Event::fake();

$registrationService->register($dataCausingRollback);
Event::assertDispatched("eloquent.created: App\User"); // fired within transaction.
Event::assertNotDispatched(Registered::class); // event fired after transaction is committed.

This isn't the only way of handling this problem, but all I'm saying is we never encounter this problem using the approach I've described. Plus the added benefit is when we're saving models via seeders or factories, we never get a bunch of business logic (non-Eloquent) events being fired.

[TitasGailius]

You're talking how to overcome this problem however it does not change the fact that when the created event is fired, it's very intuitive to expect that the record was inserted into the database and exists there.

[lindyhopchris]

I understand your perspective, but from my perspective this isn't a problem. It's a documentation issue around the explanation of what Eloquent events are and what they should be used for.

eloquent.created to me means an Eloquent model was created, not it was created in the database. Because I only get that event if I create via a model rather than directly in the database.

Architecturally, to send a welcome email (the example that kicked this off) I need to know that the user registered, not exists in the database. The two are completely different things. The only legit actions to take when Eloquent is doing something in the database is to do other database things - which are therefore covered by the transactions.

So my suggestion is this issue is solved by improving the Laravel docs to make it clearer what Eloquent events should be used for. I'd suspect that's the Laravel approach considering there's a Registered user event, Passport has token creation events rather than binding to the Eloquent model events, etc etc.

That's my perspective. This is an ideas issue, so my input is that I don't think Laravel needs to implement the idea - it just needs to improve its docs around Eloquent events.

[4n70w4]

Another example: In a Laravel Nova (from the authors of the Laravel Framework) documentation recommends using events of eloquent models.
https://nova.laravel.com/docs/2.0/resources/#resource-events

[mfn]

I think it's important that, whether an event is supposed to be transaction safe or not, has to be a developers deliberate decision. I think the arguments brought forward by @lindyhopchris well explain this.

In other words:

• only application specific events (App\Events)
• and opt-in (for example a marker interface on the event class)

It would be "rather easy" to do this for eloquent (or any framework-provided) events: you listen for the internal event and dispatch your own, with the marked interface.

But no matter if Laravel would natively support this or through a package: whether an event will be queued when transactions are active or not must be opt-in, this is a very important aspect.

(sidenote: https://github.com/fntneves/laravel-transactional-events does support this)

[rs-sliske]

i think this is just something to be aware of when using db transactions, and doesnt really feel like something that could be solved generally (any "fix" is going to break other uses).

the only solution thats really coming to mind is having something like the event fake that can be toggled to queue up events rather than firing immediately. so the developer can easily 'pause' and 'resume' event propagation.

[4n70w4]

@rs-sliske at least 2 things can be done:

• add wasCommited flag to the model
• add an array with the current committed attributes of the model

Based on this flag, change the behavior of the framework for non-commited models.

[mfn]

The model-centric approach is short sighted as you might want non-Eloquent transactional events as well.

[mvanduijker]

Same problem arises when sending mails within a transaction. I created a package for that as well (https://github.com/mvanduijker/laravel-transactional-mails). It would be nice to have some mechanics to handle these cases within the framework. Especially when you queue stuff you can also get into nasty race condition issues.

For example:

DB::beginTransaction();
$model->update(['some' => 'stuff']);
Maill::to('some-user@example.com')->queue(new SomeEmail($model));
// do something that takes time, or the database server is kind a busy
sleep(2);

DB::commit();

The queue will pick up the mail before the database has committed the transaction. If the mail depends on the updated model state things will get nasty. Same thing for queued event listeners, I guess.

[lindyhopchris]

If the mail depends on the updated model state...

Surely that means Mail::to needs to be called after DB::commit()?

That's what I'm struggling to understand with this whole issue! If you're not using the database queue driver, anything that's queued needs to happen after the database transaction has been committed, not before.

If you're using the database queue driver, the above example would be fine. But the code isn't very robust because if you swapped to using the Redis queue driver, you'd start getting problems.

So your example should be:

DB::beginTransaction();
$model->update(['some' => 'stuff']);
DB::commit();

// database now in known state, dispatch to the queue.
Maill::to('some-user@example.com')->queue(new SomeEmail($model));

Yes some sort of transaction aware event dispatcher would help with this, but only really the developer will know what events/jobs they want within the transaction, and those that they want after the transaction commits. And even if you did add that kind of dispatching, how about events that need to happen between transactions?!

Which overall is why I feel this is not a problem of the framework: it's about the developer sequencing things that match the use-case of their application. Which is why IMHO transactional aware event dispatching should be the realm of an optional package, not in the framework.

[mfn]

That's what I'm struggling to understand

• the transaction might happen well outside the actual "code scope", i.e. POST API or GraphQL mutations requests automatically wrapped in transactions and who-knows-what-code gets actually executed
• maybe your business logic should not care whether it's running in a transaction or not
• the decision to send the email may depend on state not available "outside" the transaction due to the architecture of the application

Which overall is why I feel this is not a problem of the framework

"Problem" is a hard word.

But there's certainly evidence in this discussion that some kind of support for such cases would be very useful.

Usually: the bigger and complex your Laravel application grows, the more this becomes an issue.

[deleugpn]

I feel like I should post this again. The context of an Eloquent event is subjective to change. Eloquent events should not be transaction aware and you should use your own event to represent your domain context instead.

[mfn]

Eloquent events should not be transaction aware and you should use your own event to represent your domain context instead.

FTR, this is exactly what the package I mentioned in #1441 (comment) provides

[TitasGailius]

Well, this discussion kinda went off-topic because initially, I was talking only about Eloquent events.

In my honest opinion, it makes sense to dispatch created, updated and saved Eloquent events only after the transaction is successfully committed.

Either way, if a project really needs to support this kind of feature, it is a small and easy change to add so probably it's not worth introducing this kind of a breaking change to the core.

[mfn]

Hello everyone,

based on an existing package and with permission of its original author, I've drafted a possible solution to tackle the "transactional events" problem.

At the moment it's a draft PR in my Laravel forked repo to first gather feedback from the community before approaching to create an official PR, to give things time to unfold / develop.

I invite everyone to participate in mfn/laravel-framework#1 and share your feedback.

Thank you!