fix begin trasaction after reconnect #3679

kalinin-k-a · 2019-09-20T10:06:15Z

Q	A
Type	bug
BC Break	no
Fixed issues

Summary

This is a small fix if we lose the connection and work with transactions after reconnect.
The code examples below demonstrate a problem:

/** Doctrine\DBAL\Connection $co */
$co->executeQuery('CREATE TABLE if not exists test11(test int not null)');
$co->executeQuery('truncate table test11');
$co->executeQuery('insert into test11 values (1)');
$co->beginTransaction();
$co->executeQuery('insert into test11 values (2)');
$v = $co->fetchAll('select * from test11');
$co->close(); // here we lost a connection or closed it for some reason
$co->beginTransaction(); /* p1 */
$co->executeQuery('insert into test11 values (33)');
$co->rollback(); /* p2 */
$v = $co->fetchAll('select * from test11'); // I expect the value 33 will not be selected from the table because it was rollbacked. But the transaction was not started on the line "p1" because of nestingLevel had not beed reset after open new connection.

morozov · 2019-09-20T19:41:16Z

@kalinin-k-a please turn the scenario in the description into a test.

kalinin-k-a · 2019-09-20T20:55:37Z

@kalinin-k-a please turn the scenario in the description into a test.

done

greg0ire · 2019-09-21T09:15:05Z

Why are there 2 PRs for this? #3679 looks the same, but on another branch and without tests, which one is the right one?

greg0ire · 2019-09-21T09:20:29Z

Please squash your commits

fix begin trasaction ofter reconnect

Please explain in your commit message what was wrong, and why your changes fix that situation. Maybe

Reset transaction nesting level on connection loss

When the connection is lost, subsequent transactions will no longer be
nested because they are executed in a brand new session. Our internal
representation of the nesting should take this into account.

kalinin-k-a · 2019-09-21T11:27:14Z

Why are there 2 PRs for this? #3679 looks the same, but on another branch and without tests, which one is the right one?

Another is into v2.5. I'll also write tests

greg0ire · 2019-09-21T11:47:09Z

Please associate your email address with your Github account, or change the email in your commits to an address already associated with it.

kalinin-k-a · 2019-09-21T12:01:09Z

commits are squashed, email is changed. Thank you for reminding !

kalinin-k-a · 2019-09-21T12:51:22Z

pushed again because mysql server had gone away during functional tests previously

morozov · 2019-09-21T17:28:26Z

tests/Doctrine/Tests/DBAL/Functional/ConnectionTest.php

+        $this->connection->beginTransaction(); // should connect, reset nesting level and increase it once
+        self::assertEquals(1, $this->connection->getTransactionNestingLevel());
+        $this->connection->commit();
+        self::assertEquals(0, $this->connection->getTransactionNestingLevel());


Please use the scenario from the description. The test as it's written now does not reflect the actual issue.

I’m not sure that scenario from description is good for test. It just shows one case that may be affected by nesting level issue. Also the result from the description may happen if for example commit works wrong.

I fixed nesting level reset, so i tested the same.

Do you insist on change test?

Without that case, it's not clear why we want to reset the transaction nesting level after establishing a connection. The level is an internal implementation detail of the DBAL, it doesn't have to be tested in a functional way but I'd rather have a functional case which relies on it. Even if it's just one case.

Ok.
Strange, but Insert statement fails the test now with "Table not found". Seems like table exist only before closing connection in testing environment. I'll look again a little later.

Fixed. It was sqlite connection with memory=true. Please review once more

tests/Doctrine/Tests/DBAL/Functional/ConnectionTest.php

morozov · 2019-10-07T17:09:27Z

tests/Doctrine/Tests/DBAL/Functional/ConnectionTest.php

+        $this->connection->beginTransaction();
+        $connection->close(); // connection closed in runtime (for example if lost or another application logic)
+
+        $connection->beginTransaction(); // should connect, reset nesting level and increase it once


The comment doesn't seem relevant to what the test is about.

"Should realy start the transaction, and the rollback() call below realy leads to rollback it " - better?

The transaction nesting level could be checked here too.

"Should realy start the transaction, and the rollback() call below realy leads to rollback it " - better?

Just remove the comment. The previous one says enough.

Hm… it's still there.

Hm, it really was. Forgot to commit, sorry. Now it's realy done)

SenseException · 2019-10-10T21:37:33Z

tests/Doctrine/Tests/DBAL/Functional/ConnectionTest.php

+        $connection->executeQuery('insert into test_nesting values (33)');
+        $connection->rollback();
+
+        self::assertEquals(0, $connection->fetchColumn('select count(*) from test_nesting'));


Should the transaction nesting level be checked here too?

Not really. The fact that the transaction was successfully rolled back is enough. The nesting level is an implementation detail in this case.

Should the transaction nesting level be checked here too?

The exact values of nestingLevel is completely tested in testTransactionNestingBehavior()

SenseException · 2019-10-10T21:39:28Z

tests/Doctrine/Tests/DBAL/Functional/ConnectionTest.php

+        $this->connection->beginTransaction();
+        $connection->close(); // connection closed in runtime (for example if lost or another application logic)
+
+        $connection->beginTransaction(); // should connect, reset nesting level and increase it once


The transaction nesting level could be checked here too.

When the connection is lost or is closed, subsequent transaction will no longer be nested because they started in a brand new session. Our internal representation of the nesting shold take this into account

Transaction nesting level is tested in another test.

morozov · 2019-10-14T15:00:11Z

Thank you @kalinin-k-a.

@morozov

Release v2.9.3 [![Build Status](https://travis-ci.org/doctrine/dbal.svg?branch=v2.9.3)](https://travis-ci.org/doctrine/dbal) This release fixes regressions introduced in previous releases and other bugs. - Total issues resolved: **5** - Total pull requests resolved: **14** - Total contributors: **9** **Regressions** - [3686: Fixed query result caching when `FetchMode::COLUMN` is used](doctrine#3686) thanks to @morozov and @Junker - [3456: Compare type class when comparing columns.](doctrine#3456) thanks to @garret-gunter and @cs278 **Other bugs** - [3679: fix begin trasaction after reconnect](doctrine#3679) thanks to @kalinin-k-a - [3547: Default column expressions do not work on SQL Server](doctrine#3547) thanks to @morozov - [3420: Index length can be a `string`: ensure that it is an integer when read by the `MySqlSchemaManager`](doctrine#3420) thanks to @leofeyer **CI improvements and maintenance** - [3702: Updated SQL Server extensions to fix build failures on PHP 7.4](doctrine#3702) thanks to @morozov - [3662: Marked connection exception test incomplete on MySQL 8](doctrine#3662) thanks to @morozov - [3622: Switched from ibmcom/db2express-c to ibmcom/db2](doctrine#3622) thanks to @morozov - [3465: Replaced MySQL 5.7 installed from a PPA with an official Docker image](doctrine#3465) thanks to @morozov - [3454: CI: Test against PHP 7.4snapshot instead of nightly (8.0)](doctrine#3454) thanks to @Majkl578 - [3452: Fixed AppVeyor build configuration and the issue on SQL Server](doctrine#3452) thanks to @morozov and @Majkl578 - [3447: Replaced custom docker image for PostgreSQL with the official one](doctrine#3447) thanks to @morozov - [3407: CI: Test against MySQL 8.0 on Travis](doctrine#3407) thanks to @morozov **PHP 7.4 support** - [3642: Fixed test failures on PHP 7.4](doctrine#3642) thanks to @morozov # gpg: Signature made Sat Nov 2 23:20:42 2019 # gpg: using RSA key 374EADAF543AE995 # gpg: Can't check signature: public key not found

kalinin-k-a changed the title ~~fix begin trasaction ofter reconnect~~ fix begin trasaction after reconnect Sep 20, 2019

morozov added Bug Connecting Transactions labels Sep 20, 2019

kalinin-k-a force-pushed the fix_begin_transaction_after_reconnect branch from f3d74d2 to d4bfeab Compare September 21, 2019 11:43

greg0ire previously approved these changes Sep 21, 2019

View reviewed changes

kalinin-k-a dismissed greg0ire’s stale review via e88cf34 September 21, 2019 11:57

kalinin-k-a force-pushed the fix_begin_transaction_after_reconnect branch from d4bfeab to e88cf34 Compare September 21, 2019 11:57

greg0ire previously approved these changes Sep 21, 2019

View reviewed changes

kalinin-k-a dismissed greg0ire’s stale review via 58bd4a5 September 21, 2019 12:44

kalinin-k-a force-pushed the fix_begin_transaction_after_reconnect branch from e88cf34 to 58bd4a5 Compare September 21, 2019 12:44

kalinin-k-a requested a review from greg0ire September 21, 2019 14:19

morozov reviewed Sep 21, 2019

View reviewed changes

kalinin-k-a force-pushed the fix_begin_transaction_after_reconnect branch 5 times, most recently from 4a3bc6b to 9b918da Compare September 22, 2019 09:19

kalinin-k-a force-pushed the fix_begin_transaction_after_reconnect branch 4 times, most recently from 3f2e227 to 6456e69 Compare October 2, 2019 12:58

morozov reviewed Oct 7, 2019

View reviewed changes

tests/Doctrine/Tests/DBAL/Functional/ConnectionTest.php Show resolved Hide resolved

morozov reviewed Oct 7, 2019

View reviewed changes

SenseException mentioned this pull request Oct 10, 2019

fix_begin_transaction_after_reconnect #3680

Closed

SenseException previously requested changes Oct 10, 2019

View reviewed changes

kalinin-k-a force-pushed the fix_begin_transaction_after_reconnect branch from 6456e69 to 3df789f Compare October 11, 2019 08:29

morozov previously approved these changes Oct 11, 2019

View reviewed changes

Reset transaction nesting level on connection loss.

b550879

When the connection is lost or is closed, subsequent transaction will no longer be nested because they started in a brand new session. Our internal representation of the nesting shold take this into account

kalinin-k-a dismissed morozov’s stale review via b550879 October 14, 2019 07:58

kalinin-k-a force-pushed the fix_begin_transaction_after_reconnect branch from 3df789f to b550879 Compare October 14, 2019 07:58

morozov approved these changes Oct 14, 2019

View reviewed changes

morozov merged commit 8012286 into doctrine:master Oct 14, 2019

morozov added a commit that referenced this pull request Oct 14, 2019

Merge branch 'bpo/2.9/#3679' into 2.9

24170d3

morozov added this to the 2.9.3 milestone Oct 14, 2019

morozov self-assigned this Oct 14, 2019

nanasess mentioned this pull request Nov 18, 2019

Doctrine DBAL 2.9.3 Support EC-CUBE/ec-cube#4387

Closed

morozov mentioned this pull request Nov 24, 2019

Remove temporary SQLite file on teardown #3745

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix begin trasaction after reconnect #3679

fix begin trasaction after reconnect #3679

kalinin-k-a commented Sep 20, 2019 •

edited by morozov

Loading

morozov commented Sep 20, 2019 •

edited

Loading

kalinin-k-a commented Sep 20, 2019

greg0ire commented Sep 21, 2019

greg0ire commented Sep 21, 2019 •

edited

Loading

kalinin-k-a commented Sep 21, 2019

greg0ire commented Sep 21, 2019

kalinin-k-a commented Sep 21, 2019

kalinin-k-a commented Sep 21, 2019

morozov Sep 21, 2019

kalinin-k-a Sep 21, 2019

morozov Sep 21, 2019

kalinin-k-a Sep 22, 2019

kalinin-k-a Oct 2, 2019

morozov Oct 7, 2019

kalinin-k-a Oct 7, 2019 •

edited

Loading

SenseException Oct 10, 2019

morozov Oct 11, 2019

kalinin-k-a Oct 11, 2019

morozov Oct 11, 2019

kalinin-k-a Oct 14, 2019

SenseException Oct 10, 2019

morozov Oct 11, 2019

kalinin-k-a Oct 11, 2019

SenseException Oct 10, 2019

morozov commented Oct 14, 2019

fix begin trasaction after reconnect #3679

fix begin trasaction after reconnect #3679

Conversation

kalinin-k-a commented Sep 20, 2019 • edited by morozov Loading

Summary

morozov commented Sep 20, 2019 • edited Loading

kalinin-k-a commented Sep 20, 2019

greg0ire commented Sep 21, 2019

greg0ire commented Sep 21, 2019 • edited Loading

kalinin-k-a commented Sep 21, 2019

greg0ire commented Sep 21, 2019

kalinin-k-a commented Sep 21, 2019

kalinin-k-a commented Sep 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalinin-k-a Oct 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morozov commented Oct 14, 2019

kalinin-k-a commented Sep 20, 2019 •

edited by morozov

Loading

morozov commented Sep 20, 2019 •

edited

Loading

greg0ire commented Sep 21, 2019 •

edited

Loading

kalinin-k-a Oct 7, 2019 •

edited

Loading