Don't attempt to rollback when connections are lost#7240
Conversation
It's useless to try to perform a rollback when there's no active connection, not to mention that the use of `NoActiveTransaction` exceptions masks the real underlying issue. This attempts to fix the problem by detecting connection issues and bypassing rollbacks. Signed-off-by: Luís Cobucci <[email protected]>
morozov
left a comment
morozov
left a comment
There was a problem hiding this comment.
Looks good in general, just one question.
|
|
||
| $successful = true; | ||
| } catch (ConnectionLost $connectionLost) { | ||
| // Catching here only to be able to prevent a rollback attempt |
There was a problem hiding this comment.
Isn't this dangerous when the exception is comming from another connection, ie. not rollbacking the actual transaction?
There was a problem hiding this comment.
I believe that would be applicable to any of the captured exceptions, though.
Nevertheless, in which scenario would you imagine someone starting a transaction on the connection#1 and performing queries on the connection#2?
There was a problem hiding this comment.
Any SQL query made on another connection (and throwing ConnectionLost exception) inside the transaction. Nothing rare. I belive the caught exception must be checked for the connection instance.
There was a problem hiding this comment.
IMO it's quite odd to send SQL query to another connection inside a transaction...
If anyone need to fetch data from source A to modify stuff on source B, I'd expect only the changes in source B to happen in the transaction...
There was a problem hiding this comment.
Nevertheless, feel free to send a PR with changes...
There was a problem hiding this comment.
I mean, with any other DBAL exception that we handle here, we'd have the same problem, wouldn't we?
There was a problem hiding this comment.
In theory yes, but a) not many functions accepts callback in critical sections, b) transaction must be robust. I pointed out a real issue and in atk4/data we already support multiple cross-DB operations, ie. my feedback is comming from existing usecase.
4 participants
Summary
It's useless to try to perform a rollback when there's no active connection, not to mention that the use of
NoActiveTransactionexceptions masks the real underlying issue.This attempts to fix the problem by detecting connection issues and bypassing rollbacks.