-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix incorrect transactional()
handling when DB auto-rolled back the transaction
#6545
base: 3.9.x
Are you sure you want to change the base?
Conversation
src/Connection.php
Outdated
} finally { | ||
if (! $successful) { | ||
$this->rollBack(); | ||
} | ||
} | ||
|
||
$this->commit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if commit
throws for some other reason, before anything is transmitted to the database?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you describe some scenario you have in mind so I can cover it with test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, there are throws statements:
Lines 1013 to 1026 in 220aec4
throw NoActiveTransaction::new(); | |
} | |
if ($this->isRollbackOnly) { | |
throw CommitFailedRollbackOnly::new(); | |
} | |
$connection = $this->connect(); | |
if ($this->transactionNestingLevel === 1) { | |
try { | |
$connection->commit(); | |
} catch (Driver\Exception $e) { | |
throw $this->convertException($e); |
I don't know if there is a realistic scenario in which they can throw when commit
is called from the method you are modifying, before the transaction is actually committed, but that's what I was thinking about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Those should not happen but I have adjusted the flow and covered with unit test should it happen.
if ($shouldRollback) { | ||
$this->rollBack(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not do something similar to what I did in doctrine/orm@b6137c8 and use isTransactionActive()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot about that API, should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should happen in this scenario?
$connection->beginTransaction();
$connection->transactional(some function call here);
$connection->commit();
I'm not familiar with nested transactions, but if it's possible to nest them then this will return true because of the outer transaction, although the outer transaction will have been property closed. Also, why not reuse the previous try
/ finally
block just like I did in ORM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nested transactions should be covered by 4 tests.
I merged try blocks, it makes sense and is nice with the isTransactionActive
API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now the code looks like it is going to wrongly revert the outer transaction after successfully committing the inner one, just because the outer one is active (as it should)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm I'm confused now. Will take a break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add a check of nesting level but the green test suite somehow implies it won't happen at all anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It indeed tried to rollback the transaction. I have reverted to a previous version and added a test to cover the case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused now. Are you saying that the bug exists, and that you reproduced it? I don't see any check on the transaction nesting level, which does sound exactly like what would be needed here, so why is the tests suite mostly green (it's red on Oracle, do you know why?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's red on Oracle, do you know why
oracle and db2 require select...
to have also from
clause. Fixed.
Are you saying that the bug exists, and that you reproduced it
Yes, the bug would have been introduced with 88879b3. I covered it with testNestedTransactionWalkthrough()
so we know it passes.
When doing commit()
, we need to rollback the transaction only if something failed outside the database #6545 (comment).
Since we need to have rollback()
in finally
, we need to use boolean flag to know whether to trigger it.
We can't use isTransactionActive()
since it only checks the nesting level. isTransactionActive()
can actually be renamed to something isTransactionNested()
.
It seems there are CI jobs failing. Please take a look at this guide for more on how to handle those. |
Thanks, just did not notice that basic stuff, u were faster 🙃 fixed. |
71e5b43
to
9879edf
Compare
ebc4aa1
to
b512770
Compare
$this->releaseSavepoint($this->_getNestedTransactionSavePointName()); | ||
} | ||
} finally { | ||
$this->updateTransactionStateAfterCommit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the above lines are successful but something goes wrong here, then $shouldRollback
will be true
(right?) although no rollback should happen.
That's why I was expecting some code that takes a look at the transaction nesting level before and after the commit. If the transaction level decreased all by itself.
Looking at the code though, I don't see how it could fail, so I guess this is OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unclear to me why you are calling this function inside a finally
block. If the commit fails, why should the transaction nesting level decrease?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the commit fails, why should the transaction nesting level decrease?
We need to cleanup the transaction state. Otherwise isTransactionActive()
will still report true to other functions (e.g. in disconnect()
) which is not true actually. It is covered by UniqueConstraintViolationsTest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should it no longer be considered active? If COMMIT
fails, then the transaction will be "aborted", but it won't be auto-rolled-back, right? So to me, I'd say the transaction is still going on, but it is aborted and unusable, and to regain control, you have to use ROLLBACK TO
or ROLLBACK
.
That's what I understand after reading the last few sentences of https://www.postgresql.org/docs/current/tutorial-transactions.html anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, good point.
I came up with the solution where we can check the exception type and if it is a known case where the transaction is auto rolled back, we won't call ROLLBACK.
675d1ac
to
e9ea2b1
Compare
I think I covered the case where
and where
therefore, there does not appear to be any attempt to rollback 🤔 |
Makes sense to me. What about when |
Absolutely understand. I have covered calling
When it's true it does not satisfy the left side of the condition in order to rollback If it's false it does not satisfy the right side because
|
I think I have addressed @/derrabus' (not tagging explicitly to avoid noise) comments from previous attempt related to OCI and PDO_OCI #4846 (review) |
Please use an interactive rebase to make your commits atomic (all commits should be "revertable" / pass the build). Also, if you can split this PR, that would be great. |
Yup, though for the review process I wanted the commits to represent addressing the comments / explain the issue. Will consolidate + continue extracting. |
transactional()
handling when DB auto-rolled back the transaction
… transaction Let's get rid of There's no active transaction exception which occurs e.g. when using deferred constraints so the violation is checked at the end of the transaction and not during it. - Do not rollback only on exceptions where we know the transaction is no longer active - Introduce TransactionRolledBack exception - Transform Oracle's "transaction rolled back" exception and use the underlying one that DBAL supports
Summary
#6543 partially solved what we tried to achieve in #4846. With that
There's no active transaction
exception contains the actual exception in$previous
.Now, let's get rid of
There's no active transaction
exception which occurs e.g. when using deferred constraints so the violation is checked at the end of the transaction and not during it. Database then rolls back the transaction automatically.The current faulty flow is following:
commit()
, Constrain violation occursrollback()
There's no active transaction
exception is thrown:This PR adjusts the flow so:
commit()
, Constrain violation occursrollback()
🥳For DBAL v4 it looks like this #6546 (draft, does not contain review changes)