Convert driver exceptions when starting transactions #6812
+63
−15
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Let's get it integration-tested. Even though the unit test passes, and the code works as expected on MySQL, we want to be sure that it works the same on other platforms as well or at least know that it doesn't. |
This optimises how this test is executed by leveraging two connections and killing the one executing the tests. It also aims at making the whole conversion process available to other platforms. Signed-off-by: Luís Cobucci <[email protected]>
For long-running processes, it's possible that the wrapped connection is no longer valid and starting a transaction will lead to driver exceptions. DBAL Connection wasn't converting these, leaking internal details of the platform and not providing a consistent API (all other methods would throw a `ConnectionLost` exception for the scenario above). This ensures we do have the expected behaviour for this edge-case. Signed-off-by: Luís Cobucci <[email protected]>
lcobucci
force-pushed
the
convert-exceptions-on-begin-transaction
branch
from
5fa77a9 to
5c1255f
Compare
lcobucci
commented
Feb 25, 2025
| default => self::markTestSkipped('Unsupported test platform.'), | ||
| }; | ||
|
|
||
| $privilegedConnection = TestUtil::getPrivilegedConnection(); |
There was a problem hiding this comment.
Using priviledged connection here, assuming that's a requirement for other platforms (maybe oracle or SQL server?)
lcobucci
commented
Feb 25, 2025
|
|
||
| use const E_WARNING; | ||
|
|
||
| class TransactionTest extends FunctionalTestCase | ||
| { | ||
| public function testCommitFailure(): void | ||
| { | ||
| $this->markConnectionNotReusable(); |
There was a problem hiding this comment.
Marking as not reusable as I didn't want for a failure in one test to leak into the other
lcobucci
commented
Feb 25, 2025
Comment on lines
81
to
83
| if ( | ||
| $exception->getSQLState() === 'HY000' | ||
| && str_contains($exception->getMessage(), 'server closed the connection') | ||
| ) { | ||
| return new ConnectionLost($exception, $query); | ||
| } |
There was a problem hiding this comment.
This might only be applicable for recent PGSQL versions...
There was a problem hiding this comment.
Nah... only PDO has the SQL State set for this error :)
Ooohh I missed working with DBAL 😅
lcobucci
commented
Feb 25, 2025
lcobucci
force-pushed
the
convert-exceptions-on-begin-transaction
branch
2 times, most recently
from
75ced17 to
6e47746
Compare
This extends the functional test for connection loss detection to verify the behaviour against Postgres and ensures the ExceptionConverter for that API behaves as expected. Signed-off-by: Luís Cobucci <[email protected]>
This is required because the MySQLi API only returns a boolean result that we were simply not using. Signed-off-by: Luís Cobucci <[email protected]>
lcobucci
force-pushed
the
convert-exceptions-on-begin-transaction
branch
from
6e47746 to
432b89d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
The wrapped connection may no longer be valid for long-running processes, and starting a transaction will cause driver exceptions.
DBAL Connection wasn't converting these, leaking internal details of the platform and not providing a consistent API (all other methods would throw a
ConnectionLostexception for the scenario above).This ensures we do have the expected behaviour for this edge case.