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

[11.x] Fix how nested transaction callbacks are handled #48853

Conversation

mateusjatenee
Copy link
Contributor

@mateusjatenee mateusjatenee commented Oct 30, 2023

A problem we discovered while working on #48705 was that if you have nested transactions and you add callbacks to them, it's possible some callbacks were mistakenly removed upon failure.

The example @fntneves shared:

DB::transaction(function () use ($thirdObject, $secondObject, $firstObject) { // Transaction 1, level 1
    DB::transaction(function () use ($firstObject) { // Transaction 2, level 2
        DB::afterCommit(fn () => $firstObject->handle()); // Callback 1 for transaction 2
    });

    DB::afterCommit(fn () => $secondObject->handle()); // Callback 2 for transaction 1

    try {
        DB::transaction(function () use ($thirdObject) { // Transaction 3, level 2
            DB::afterCommit(fn () => $thirdObject->handle()); // Callback 3 for transaction 3
            throw new \Exception(); // This should only affect callback 3, not 1, even though both share the same transaction level.
        });
    } catch (\Exception) {}
});

The problem is since we only maintain a $transactions counter that goes up (when a new transaction is added) and down (when a transaction is committed, or rolled back), it's possible to have callbacks from different transactions that are on the same level intertwined.
In that example, callback 1 would be removed as soon as transaction 3 failed, because the first and last transaction share the same levels.

I discussed a few options with Taylor, such as having each transaction hold a unique identifier (uuid or just a counter that only goes up), and he suggested splitting the transactions into "ready" transactions and "pending" transactions. This PR uses the latter. Let me know if you think this makes sense..

Also, thanks @fntneves for pointing that problem on the other PR!
P.S: I was unsure whether to target 11.x or 10.x. There are no contract changes, but behavior changes slightly. Let me know and I can change it.

Pending:

  • Fix existing unit tests
  • Add unit tests
  • Add more integration tests

@mateusjatenee mateusjatenee marked this pull request as draft October 30, 2023 01:42
@mateusjatenee
Copy link
Contributor Author

This affects DatabaseTransactions used by tests — the tests are passing but I'll check if we need to change something since now transactions are divided in two "stages".

@mateusjatenee
Copy link
Contributor Author

I couldn't figure out how to properly rebase this branch to target 10.x so I opened a new PR: #48859

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

Successfully merging this pull request may close these issues.

None yet

1 participant