cakephp: [RFC] Nested transaction might be rolled back unexpectedly

This is a (multiple allowed):

bug
enhancement
feature-discussion (RFC)
CakePHP Version: 3.4.2

What you did

A simple test code:

$this->loadModel('Bookmarks');
$this->Bookmarks->connection()->transactional(function(){
    $this->Bookmarks->findOrCreate(['user_id' => -1]);
    $this->Bookmarks->Users->findOrCreate(['id' => null]);
    return false;
});

What happened

A user has been persisted.

What you expected to happen

It should be rolled back.

Why this happens

Since findOrCreate() uses transactional() internally, two transactions will be nested in the example above. The first one will be failed as an invalid users_id is passed. The second one will be succeeded as there is no such ID matching NULL. Also, note that the example code returns false whatever happens.

You may think the nested transaction will be like the following:

# transactional()
BEGIN

    # findOrCreate()
    (BEGIN)
    (ROLLBACK)

    # findOrCreate()
    (BEGIN)
    (COMMIT)

ROLLBACK

However, in fact it becomes like the following:

# transactional()
BEGIN

    # findOrCreate()
    (BEGIN)
ROLLBACK

# findOrCreate()
BEGIN
COMMIT

(ROLLBACK)

Because one rollback() call kills all nested transactions if savepoint is disabled - it means by default.

As a result, the second findOrCreate() ends up persisting a user because there is no longer nested transactions.

Solution

I was thinking about throwing an exception if some nested transaction has been rolled back. As for the example above, the first findOrCreate() should throw an exception instead of executing a ROLLBACK query:

# transactional()
BEGIN

    # findOrCreate()
    (BEGIN)
    (ROLLBACK) 
    # throw new NestedTransactionRollbackException

I would like to discuss some concerns about throwing an new exception here.

Acknowledgment

Originally, this issue has been reported by icchii in #japanese channel on our Slack. Thank you again. After that, I have investigated and found out a simple code that causes this issue. And now I am reporting it.

About this issue

Original URL
State: closed
Created 7 years ago
Reactions: 1
Comments: 15 (15 by maintainers)

Commits related to this issue

Fix nested transactions may be rolled back or committed unexpectedly Refs #10348 — committed to chinpei215/cakephp by chinpei215 7 years ago

Most upvoted comments

Maybe, we can change Connection:😒_useSavePoints to true by default in future release? Personally, I have never used the statement though.

@chinpei215 I learned the hard way how bad it is to have that setting always enabled. It caused massive deadlocks in my database, so I rather have people opt-in to te feature if they actually need it.

lorenzo on Mar 6, 2017

@chinpei215 I like the idea! While it does not solve the problem, it will at least inform the user during the testing phase that there is something wrong with their logic.

A complete solution to this problem would require a very different design of the ORM, but I think we can mitigate it with different strategies.

lorenzo on Mar 4, 2017

@lorenzo I may understand your concern a little bit. Then I have another approach and now I think this idea might be better.

// Allowed nest types
BEGIN
    (BEGIN)
    (COMMIT)
COMMIT

BEGIN
   (BEGIN)
   (COMMIT)
ROLLBACK

BEGIN
   (BEGIN)
   (ROLLBACK)
ROLLBACK

// Not allowed nest type
BEGIN
   (BEGIN)
   (ROLLBACK)
COMMIT // Error. Throw an exception.

In this idea, the rollback() call of the last type doesn’t throw an exception really, but marks the transaction as it should be failed. And if people call commit() after that, then it throws NestedTransactionRollbackException. However, if people call rollback() as failure operation, like the third example, it doesn’t throw anything.

What do you think about this idea?

chinpei215 on Mar 4, 2017

@lorenzo Thank you. I should have shown you this idea first. I think now we have some consensus for fixing the issue. As a first step, we could throw a new exception when the most outer transaction was closed, if some inconsistent operation - commit() after rollback(), was done in it. And, as the second step, we can improve the ORM to suppress insert(), update() or delete() calls if the transaction should be failed. I can do the first enhancement. I would like to put it on the top of my todo list.

The latest MySQL and SQLite also seem to support the SAVEPOINT statement. Maybe, we can change Connection::$_useSavePoints to true by default in future release? Personally, I have never used the statement though.

chinpei215 on Mar 4, 2017

Another strategy we can use after implementing you idea is to keep some state in the connection that can be used by the ORM to know when it was expecting to be inside a transaction and where it does not. This way, we can prevent calls to insert() update() or delete() if we can detect that the ORM was expecting to be inside a transaction but in reality it was closed.

Fortunately this is not a problem for postgres users 😃

lorenzo on Mar 4, 2017

For databases that don’t support nested transactions it doesn’t look like we can easily emulate nested transactions either. We don’t have many options other than an exception at that point.

markstory on Mar 4, 2017

For this case, can’t the user enable real nested transactions?

Not all databases support nested transactions.

We already discussed the problems of throwing exceptions, and in the context of the ORM they could potentially cause more harm than it would help.

I would like to hear that. What is the harm?

chinpei215 on Mar 4, 2017