Doctrine DBAL
  1. Doctrine DBAL
  2. DBAL-335

Is MasterSlaveConnection implemented correctly - seems to overwrite master connection on transaction methods?

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: None
    • 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.

        Activity

        Jonathan Ingram created issue -
        Hide
        Lars Strojny added a comment -

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

        Show
        Lars Strojny added a comment - This looks indeed like a bug. From a first glimpse the fix would be to use master, if master is already connected.
        Hide
        Benjamin Eberlei added a comment -

        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?

        Show
        Benjamin Eberlei added a comment - 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?
        Hide
        Jonathan Ingram added a comment -

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

        Show
        Jonathan Ingram added a comment - Yes I am. Does that render this moot or still a bug?
        Hide
        Benjamin Eberlei added a comment -

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

        Show
        Benjamin Eberlei added a comment - Its still a bug, but it helps to know why this happens.
        Benjamin Eberlei made changes -
        Field Original Value New Value
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 2.3 [ 10184 ]
        Resolution Fixed [ 1 ]
        Hide
        Benjamin Eberlei added a comment -

        Fixed in master and 2.3, can you test it?

        Show
        Benjamin Eberlei added a comment - Fixed in master and 2.3, can you test it?
        Hide
        Jonathan Ingram added a comment -

        Thanks for doing this. I will test it shortly.

        Show
        Jonathan Ingram added a comment - Thanks for doing this. I will test it shortly.
        Hide
        Ivan Andric added a comment -

        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

        Show
        Ivan Andric added a comment - 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

        This list may be incomplete, as errors occurred whilst retrieving source from linked applications:

        • Request to http://www.doctrine-project.org/fisheye/ failed: Error in remote call to 'FishEye 0 (http://www.doctrine-project.org/fisheye/)' (http://www.doctrine-project.org/fisheye) [AbstractRestCommand{path='/rest-service-fe/search-v1/crossRepositoryQuery', params={query=DBAL-335, expand=changesets[0:20].revisions[0:29],reviews}, methodType=GET}] : Received status code 503 (Service Temporarily Unavailable)

          People

          • Assignee:
            Benjamin Eberlei
            Reporter:
            Jonathan Ingram
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: