mirror of
https://github.com/doctrine/orm.git
synced 2026-03-23 22:42:18 +01:00
If SQLLogger or connection throws in beginTransaction, EntityManager can land in unexpected state #6604
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @Firehed on GitHub (Jan 12, 2021).
I recently experienced an issue where an exception thrown by my SQLLogger resulted in hanging transactions that persisted later into the request.
The most straightforward reproduce case is when
SQLLogger::stopQuery()was called in DBAL'sConnection::beginTransaction()an exception was thrown (which was subsequently caught by the application in such a way that permitted further processing). This occurred after the actual database transaction was opened, and after the internal counter for transaction depth was incremented.Because the transaction is started outside of
UnitOfWork::commit's try/catch block, the EntityManager isn't closed, nothing gets rolled back, etc. Subsequent calls toEntityManager::flush()are attempted, but the transaction depth is (correctly) at 2 instead of 1 for those flushes so nothing is written, and everything ends up getting backed up both within the application itself as well as the database, which continues to hold locks on any affected rows.5801474ba3/lib/Doctrine/ORM/UnitOfWork.php (L381-L383)Going waaaay back to 2009 this is an intentional change, but the reasoning behind it may no longer apply:
c3081adb09I'm not really sure what the best way to resolve this is, as it's a fairly ugly combination of events. Perhaps
begin()needs its own error handling separate from the main CRUD operations and commit.@beberlei commented on GitHub (Feb 7, 2021):
Should be a documentation change on sqllogger that exceptions are not allowed or should we handle this @morozov ?
@morozov commented on GitHub (Feb 9, 2021):
I believe it's fair to not allow the logger to throw exceptions. Otherwise, how do we handle them? Normally, exceptions should be logged but we know that the logger is broken.
The above is just common sense. If somebody thinks otherwise, please put together a test case and we can discuss it in more details.
@Firehed commented on GitHub (Feb 10, 2021):
Saying "you can't throw" doesn't seem like a useful solution here, since if you do anyway (and this is very easy if you convert notices to
ErrorExceptions) you're still left in an unknown or unexpected state that the issue describes. IfSTART TRANSACTIONthrows (regardless of how or where in the internals), I absolutely do not expect to have a transaction open at the database.Probably somewhat more to the point, if the raw
beginTransaction()call in the underlying connection throws, it seems like the exact same behavior will occur (at least with PDO). Stepping though what happens downstream ofUnitOfWork, there doesn't appear to be any error handling indbal/Connection::beginTransaction()which could cause thetransactionNestingLevelto desynchronize from the actual database state.The
beginTransaction()in UoW probably does not belong inside the existingtryblock, as that has its own weird set of implications. What I've done in the past was having it in a separate error handling block to get the counter state corrected before rethrowing the original exception, but that may not work here.@morozov commented on GitHub (Feb 10, 2021):
This is a very bad practice IMO. By converting a non-fatal error to an exception, the application makes the code execution flow dependent on the configuration (whether there is conversion or not).
Logging is not the primary concern of the component that communicates to the DB. If it fails to log something, it shouldn't affect its execution flow which throwing an exception does.
We're talking about the logger throwing an exception, not the underlying connection, right?
@Firehed commented on GitHub (Feb 10, 2021):
It's extremely common to throw upon notices or warnings; whether it's done via automatic conversion or countless
=== falsechecks is irrelevant. This conversion is done by default in many frameworks, many things that were previously errors became Exceptions in PHP 8, and there are countless libraries that exist solely to improve error handling of the standard library.What causes the exception to be thrown doesn't change the fact that the exception is not correctly handled and the library can be put in an incorrect state.
Well, I just gave an example of where that becomes a severe issue. You can claim that's not a use-case you want to support, and that's fine, but if I'd told that to my PCI auditors in a previous job I'd have been laughed out of the room. The library provides explicit support for the loggers, and thus should be prepared for the interactions that can occur from them.
I'm talking about both. I discovered it via the logger, but the issue appears to be present if the underlying connection throws too. Arguably it's an issue in DBAL rather than ORM, but given their relation it's worth considering the broader interactions.
@morozov commented on GitHub (Feb 10, 2021):
In my comment regarding the logger and error handling, I was primarily answering https://github.com/doctrine/orm/issues/8423#issuecomment-774646138. If there is improper handling of errors originating in the underlying DB connection, it definitely looks like something that should be fixed/improved in the DBAL.
@Firehed commented on GitHub (Feb 10, 2021):
Fair enough, but I'll maintain my position that if the library offers support for logger integration, they too should not be able to put the connection in an invalid state. If the proposal to dealing with that is catching and discarding any exceptions, and documenting that behavior, I guess that's ok though I think it could be done better.
I'm more than happy to draft a pull for what I feel is a better solution, but I don't want to invest the time to do so if it'll be rejected out of hand. Looking at the underlying connection logic I don't think it's terribly difficult but probably requires building a lot of additional test cases.
@morozov commented on GitHub (Feb 10, 2021):
This is achieved by not allowing the logger to throw an exception.
No. The proposal is to document that the implementors of the logger interface must not throw any exceptions. If they do, they violate the API and thereby are not supported.
If the PR will catch and discard the exceptions thrown by the logger, I won't be in favor of this change. This way, we'd put the burden of handling logging exceptions on the DBAL while it doesn't own the logging logic.