[DBAL-335] Is MasterSlaveConnection implemented correctly - seems to overwrite master connection on transaction methods? Created: 31/Aug/12  Updated: 22/Sep/12  Resolved: 17/Sep/12

Status: Resolved
Project: Doctrine DBAL
Component/s: None
Affects Version/s: 2.3
Fix Version/s: 2.3

Type: Bug Priority: Major
Reporter: Jonathan Ingram Assignee: Benjamin Eberlei
Resolution: Fixed Votes: 0
Labels: None


 Description   

Forgive me to doubt, but I think there may be a bug in MasterSlaveConnection.

It's easier to understand what I'm saying by debugging and tracing the flow, but I'll illustrate it with gists.

First, here is a simple service method to create a user. It opens up a transaction, persists the user, commits and returns. On error, if there is an active transaction, rollback. Here is the gist:

https://gist.github.com/3547674

The "$conn->beginTransaction();" line is where we trace through (the remainder of the service method is now irrelevant). Looking into MasterSlaveConnection.php, we see the method tries to connect to the master connection (call this point ###):

https://gist.github.com/3547720

Now looking in the next gist, we see what happens when "$this->connect('master');" is called. At this point it's not that interesting, the internal "$this->_conn" property is set to "master".

https://gist.github.com/3547750

Now here lies the bug I believe. "parent::beginTransaction();" is called. When looking into this method, we see that another call is made to connect but this time without "master" as the argument (i.e. connect to slave). This call to connect is made before incrementing the transaction nesting level.

https://gist.github.com/3547808

Now, I won't do another gist for "MasterSlaveConnection::connect", but if you refer to the file at line 13 https://gist.github.com/3547750#file_master_slave_connection.php, you will see that it checks the transaction nesting level and if it is there, forces master. However, we don't increment the level until after the method returns, so the slave is used. Ultimately, this results in the internal "$this->_conn" property set to the "slave" connection which violates our original action at ### above where we said we want to connect to "master".

Am I missing something here? Here is a gist the is a basic attempt at fixing this one method. It simply copies the code from the parent method except does not connect twice. I believe the same would have to occur for all the other methods unless it can be fixed once at the "MasterSlaveConnection::connect" level.

https://gist.github.com/3547880

I've just fleshed out "beginTransaction", "commit" and "rollBack" in "MasterSlaveConnection" by basically copying and pasting the code from the parent class and for my failing use case, this fixes the issue. However, it did require updating "Connection" slightly so that I had access to some private variables.

 Comments   
Comment by Lars Strojny [ 31/Aug/12 ]

This looks indeed like a bug. From a first glimpse the fix would be to use master, if master is already connected.

Comment by Benjamin Eberlei [ 05/Sep/12 ]

This only happens when "keepSlave" = true, because then the master is not written into the slave property aswell:

   } else {
     $this->connections['slave'] = $this->_conn = $this->connectTo($connectionName);
   }

Are you using keepSlave = true?

Comment by Jonathan Ingram [ 05/Sep/12 ]

Yes I am. Does that render this moot or still a bug?

Comment by Benjamin Eberlei [ 06/Sep/12 ]

Its still a bug, but it helps to know why this happens.

Comment by Benjamin Eberlei [ 17/Sep/12 ]

Fixed in master and 2.3, can you test it?

Comment by Jonathan Ingram [ 19/Sep/12 ]

Thanks for doing this. I will test it shortly.

Comment by Ivan Andric [ 22/Sep/12 ]

Hi,

not sure if you managed to test this but now test on mysql database fails with results below.
Probably some typo.

There was 1 error:

1) Doctrine\Tests\DBAL\Functional\MasterSlaveConnectionTest::testKeepSlaveBeginTransactionStaysOnMaster
Exception: [Doctrine\DBAL\DBALException] An exception occurred while executing 'INSERT INTO master_slave_table (test_int) VALUES ' with params

{"1":30}

:

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '30' for key 'PRIMARY'

With queries:
2. SQL: 'CREATE TABLE master_slave_table (test_int INT NOT NULL, PRIMARY KEY(test_int)) DEFAULT CHARACTER SET utf8 COLLATE utf8_unicode_ci ENGINE = InnoDB' Params:

Trace:
/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:793
/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:231
/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:539
/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:285
/home/ivan/git/dbal2/dbal/tests/Doctrine/Tests/DBAL/Functional/MasterSlaveConnectionTest.php:92

/home/ivan/git/dbal2/dbal/tests/Doctrine/Tests/DbalFunctionalTestCase.php:73
/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:793
/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:231
/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:539
/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:285
/home/ivan/git/dbal2/dbal/tests/Doctrine/Tests/DBAL/Functional/MasterSlaveConnectionTest.php:92

Caused by
Doctrine\DBAL\DBALException: An exception occurred while executing 'INSERT INTO master_slave_table (test_int) VALUES ' with params

{"1":30}

:

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '30' for key 'PRIMARY'

/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/DBALException.php:47
/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:786
/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:231
/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:539
/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:285
/home/ivan/git/dbal2/dbal/tests/Doctrine/Tests/DBAL/Functional/MasterSlaveConnectionTest.php:92

Caused by
PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '30' for key 'PRIMARY'

/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:786
/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:231
/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connection.php:539
/home/ivan/git/dbal2/dbal/lib/Doctrine/DBAL/Connections/MasterSlaveConnection.php:285
/home/ivan/git/dbal2/dbal/tests/Doctrine/Tests/DBAL/Functional/MasterSlaveConnectionTest.php:92

Generated at Wed May 22 15:06:48 UTC 2013 using JIRA 5.2.7#850-sha1:b2af0c8dc8537b36121c6a579fabbdf79fc919e5.