fntneves / laravel-transactional-events

Transaction-aware Event Dispatcher for Laravel
MIT License
314 stars 28 forks source link

Nested multiple connections problems. #23

Closed ktanakaj closed 5 years ago

ktanakaj commented 5 years ago

Hi! I have many databases in one app. But there's something wrong with laravel-transactional-events when nested multiple connections. I report these cases.

OK (Single connection)

\DB::connection('logdb')->transaction(function() {
    event(new \App\Events\EventA);
    event(new \App\Events\EventB);
});

Of course it is no problems. I got both events.

NG (Exception when nested multiple connections)

\DB::connection('logdb')->transaction(function() {
    \DB::transaction(function() {
        event(new \App\Events\EventA);
    });
    event(new \App\Events\EventB);
});

I got exception below.

ErrorException: count(): Parameter must be an array or an object that implements Countable in /vagrant/server/vendor/fntneves/laravel-transactional-events/src/Neves/Events/TransactionalDispatcher.php:152
Stack trace:
#0 [internal function]: Illuminate\Foundation\Bootstrap\HandleExceptions->handleError(2, 'count(): Parame...', '/vagrant/server...', 152, Array)
#1 /vagrant/server/vendor/fntneves/laravel-transactional-events/src/Neves/Events/TransactionalDispatcher.php(152): count(NULL)

NG (Inner event not fired when nested multiple connections)

\DB::connection('logdb')->transaction(function() {
    event(new \App\Events\EventB);
    \DB::transaction(function() {
        event(new \App\Events\EventA);
    });
});

I got EventB only. EventA is not fired.

My Environment

ktanakaj commented 5 years ago

I think there are some solutions.

  1. All events fire after root transaction committed.
  2. Each events fire after each transaction that event including committed.
  3. Add $connection properties to TransactionalEvent like Model, and each events work for own connection.
ktanakaj commented 5 years ago

I might fixed my local source in the following way.

I don't know whether this is the best...

fntneves commented 5 years ago

Hi @ktanakaj,

Will take a look into this issue. Thank you for opening it!

azeos commented 5 years ago

I'm getting "count(): Parameter must be an array or an object that implements Countable" too.

Environment:

I think the problem has something to do with PHP 7.2

TransactionalDispatcher.php

protected function addPendingEvent($connection, $event, $payload)
{
    $connectionId = $connection->getName();
    $transactionLevel = $this->transactionLevel;
    $eventData = [
        'event' => $event,
        'payload' => is_object($payload) ? clone $payload : $payload,
    ];
    $transactionLevelEvents = &$this->pendingEvents[$connectionId][$transactionLevel];
    $transactionLevelEvents[count($transactionLevelEvents) - 1][] = $eventData;
}

In my case $transactionLevelEvents is null, that's triggering the exception.

Code:

A simplified version of my code. I'm using the laravel tenancy package and the laravel permission package.

public static function store(StoreUser $request, UserTenant $user = null)
{
    $db = DB::connection('tenant');
    $db->beginTransaction();
    try {
        // Fields
        $user = $user ?: new self;
        $user->last_name = $request->input('last_name');
        $user->first_name = $request->input('first_name');
        $user->save();

        // Roles
        $oldRoles = $user->roles->pluck('id')->toArray();
        $newRoles = array_map('intval', $request->input('roles', []));
        $updatedRoles = array_merge(array_diff($oldRoles, $newRoles), array_diff($newRoles, $oldRoles));
        $user->syncRoles($newRoles);
        event(new UserTenantRolesChanged($user, $updatedRoles));

        // Permissions
        $user->syncPermissions($request->input('permissions', []));

        // Commit
        $db->commit();

        return $user;
    } catch (\Exception $e) {
        $db->rollback();

        return false;
    }
}
fntneves commented 5 years ago

@azeos Can you please show an example snippet that raises this exception?

Is it the same as @ktanakaj showed above?

azeos commented 5 years ago

@fntneves I updated my comment. Could $transactionLevelEvents be null? Or something is wrong before that?

azeos commented 5 years ago

@fntneves any updates on this?

fntneves commented 5 years ago

@azeos I am working on this issue. Hopefully, will push a new release during this weekend.

azeos commented 5 years ago

@fntneves thanks! Just to know if I need to search for another alternative.

fntneves commented 5 years ago

@azeos Just to contextualise, the solution of this issue is more complex than it seems to be.

The thing is that dispatched events, in Laravel, are not related to connections. This package introduces that responsibility. Therefore, the logic to handle this has lots of edge cases in several scenarios that I must foresee.

Also, the fix must work for SAVEPOINTS scenarios, that is, nested transactions within the same connection and keep forcing that events are dispatched following the same order they were registered.

Give me some time to organise this, most of the code will be changed.

Thank you, of course, for your patience!

fntneves commented 5 years ago

@azeos I will follow one of the proposed solutions from @ktanakaj. Events will be triggered only after the root transaction commits, which means that even when the inner transaction succeeds, if the root transaction fails, no event is fired at all. For such reason, I do not recommend to rely on events dispatched on nested transactions on distinct database services.

In detail, the concept of Transactional Event is not suitable for transactions of distinct connections, because, in the end, they are not effectively related. Considering the example given by @ktanakaj, if the transaction on logdb connection fails to commit, it will not rollback the inner transaction. At this point, should we fire the events fired by the inner transaction? If yes, application could end in an inconsistent state, because these events should not be dispatched before the previous ones (see example below, where LogConnectionEvent() is registered after CustomEvent()). Therefore, it makes no sense to handle transactions of distinct connections as if they were related, except if anything like distributed transactions (XA) is adopted.

DB::transaction(function(){
    event(new CustomEvent());

    DB::connection('log')->transaction(function() {
        event(new LogConnectionEvent());
    });

    event(new SecondCustomEvent());
})

I will update this package to fix the bug on using custom connections, but it will only dispatch events after the root transaction commits, even if the inner transaction commits.

azeos commented 5 years ago

@fntneves thanks for the update on this subject. I understand what you are saying and it seems reasonable.

fntneves commented 5 years ago

Could you please check version 1.8.1?

ktanakaj commented 5 years ago

Thanks a lot! I confirmed version 1.8.1. The events were fired after root transaction committed. Looks Good To Me :thumbsup:

fntneves commented 5 years ago

@azeos Any feedback on this?

azeos commented 5 years ago

@fntneves I'll try tomorrow, I'm working on another project right now. Will ley you know as soon as I try it. Thanks a lot!

azeos commented 5 years ago

@fntneves today I went back with this problem. Unfortunately I'm using Laravel 5.7, so I can't test v1.8.1, sorry, didn't pay attention. An update to 5.8 is planned, so I'll be back.

Thanks!

fntneves commented 5 years ago

@azeos I'm applying the fix for 1.4.x, also.

Will release a new 1.4.x version. :+1:

azeos commented 5 years ago

@fntneves just to let you know that I updated to Laravel 6 and it's working great. Thanks for the package!