Uploaded image for project: 'eZ Publish / Platform'
  1. eZ Publish / Platform
  2. EZP-26407

Save original exception when repo->commit() fails

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: High High
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.4.9, 1.7.0
    • Component/s: None
    • Labels:

      Description

      It seems quite unclear what transactional model is used within the Repository:

      • apparently many beginTransaction() calls can be called one after another, and the Repo will keep track of current transaction depth
      • when calling commit(), 2 different things happen:
        + the persistence layer commits regardless of transaction depth
        + only at depth 0 the commit events are fired
      • when calling rollback(), 2 different things happen:
        + the persistence layer rolls back regardless of transaction depth
        + only the events tied to the current transaction depth are discarded

      The main questions with this is: are nested transactions supported?

      • it seems so, as there is the concept of a transaction depth
        [EDIT *further tests seem to disprove the 3 bullet points below* ]
      • however, the persistence layer does not allow stacked transactions. To call twice commit() is a fatal error. This means they will not work
      • f.e. imagine a function X which uses a repo transaction in its body. Then a function Y which does call in order: $repo->beginTransaction(); X(); $repo->commit(); => a fatal error will ensue, as the db layer (doctrine) will not allow the 2nd commit to be executed
      • if you go look at the logic of db transactions in eZ4, you will see that the db commit() was only called when the transaction depth was 0, and the above code worked

      An ancillary question is: when rolling back, only the latest 'logical' transaction will be cancelled, whereas the db will in fact roll back all the way to the 1st begin-transaction call found. The latter behaviour is implemented by all databases afaik. This means that after a rollback() call from a nested transaction, the db will be out of transactions, whereas the repository will still think that it is in a transaction.
      This is dangerous imho, and unexpected by developers who are familiar with transactions.

      Wouldn't it make more sense to:

      • only call the db commit() when the repo commit() is called and transaction depth is 0?
      • always roll back to transaction depth 0 regardless of current transaction depth?
      • (as an alternative, make sure that the db allows partial rollbacks?)

      [EDIT *further tests seem to disprove as well the text below* ]

      As a side note: Code which used to work in eZPublish 5 and does not work any more in eZPlatform:

      • begin transaction
      • become admin (in repo)
      • edit stuff
      • become anon again
      • commit transaction

      This worked because the 'transaction' was managed as a "db transaction". The repo user did not interfere with it.

      In eZPlatform, some stuff is carried out post transaction commit.
      If that 'stuff' includes e.g. clearing cache for a location that anon user can not read, then there is a fatal error, as cache clearing involves trying to access the location...

        Activity

        Hide
        Gaetano Giunta added a comment -

        ps: example project which has been bitten by this: https://github.com/kaliop-uk/ezmigrationbundle/issues/76

        Show
        Gaetano Giunta added a comment - ps: example project which has been bitten by this: https://github.com/kaliop-uk/ezmigrationbundle/issues/76
        Hide
        Gaetano Giunta added a comment -

        ps: if you want rollbacks for nested transactions to only roll back to the latest begin() call, see how this can be achieved by using SAVEPOINT in the comment to this page: http://php.net/manual/en/pdo.begintransaction.php

        Show
        Gaetano Giunta added a comment - ps: if you want rollbacks for nested transactions to only roll back to the latest begin() call, see how this can be achieved by using SAVEPOINT in the comment to this page: http://php.net/manual/en/pdo.begintransaction.php
        Hide
        Gaetano Giunta added a comment - - edited

        pps: according to Doctrine, nested transactions are built-in into the DBAL\Connection object, and should thus work in concert with the repo transactions...

        What's very nice is that it even supports nested rollbacks. Those have to be enabled with a call to setNestTransactionsWithSavepoints() - which can be easily be set in the connection configuration yml since DoctrineBundle version 1.6.0

        Show
        Gaetano Giunta added a comment - - edited pps: according to Doctrine, nested transactions are built-in into the DBAL\Connection object, and should thus work in concert with the repo transactions... What's very nice is that it even supports nested rollbacks. Those have to be enabled with a call to setNestTransactionsWithSavepoints() - which can be easily be set in the connection configuration yml since DoctrineBundle version 1.6.0
        Hide
        Gaetano Giunta added a comment - - edited

        And here is some test code which does indeed prove that nested transactions do work.

        I am dumbfounded...
        I think that the original problem for this ticket was due to a Begin-Begin-Rollback-Begin-Commit-Commit pattern being mistaken for a Begin-Begin-Commit-Begin-Commit-Commit one. I'm not 100% sure though.

                /** @var Repository $repo */
                $repo = $this->getContainer()->get('ezpublish.api.repository');
         
                $dbHandler = $this->getContainer()->get('ezpublish.connection');
                $conn = $dbHandler->getConnection();
                //$conn->setNestTransactionsWithSavepoints(true);
         
                $repo->beginTransaction();
                $repo->commit();
                $repo->beginTransaction();
                $repo->rollback();
                echo "BCBR: OK\n";
         
                $repo->beginTransaction();
                $repo->rollback();
                $repo->beginTransaction();
                $repo->commit();
                echo "BRBC: OK\n";
         
                $repo->beginTransaction();
                $repo->beginTransaction();
                $repo->commit();
                $repo->commit();
                echo "BBCC: OK\n";
         
                $repo->beginTransaction();
                $repo->beginTransaction();
                $repo->commit();
                $repo->rollback();
                echo "BBCR: OK\n";
         
                $repo->beginTransaction();
                $repo->beginTransaction();
                $repo->rollback();
                $repo->rollback();
                echo "BBRR: OK\n";
         
                // will throw unless partial rollbacks (savepoints) are enabled
                $repo->beginTransaction();
                $repo->beginTransaction();
                $repo->rollback();
                $repo->beginTransaction();
                $repo->commit();
                $repo->commit();
                echo "BBRBCC: OK\n";
         
                // will throw unless partial rollbacks (savepoints) are enabled
                $repo->beginTransaction();
                $repo->beginTransaction();
                $repo->rollback();
                $repo->commit();
                echo "BBRC: OK\n";
        

        Long story short:

        • nested transactions are supported
        • we could probably recommend that users set the nested_transactions to on by default in their doctrine configuration
        Show
        Gaetano Giunta added a comment - - edited And here is some test code which does indeed prove that nested transactions do work. I am dumbfounded... I think that the original problem for this ticket was due to a Begin-Begin-Rollback-Begin-Commit-Commit pattern being mistaken for a Begin-Begin-Commit-Begin-Commit-Commit one. I'm not 100% sure though. /** @var Repository $repo */ $repo = $this->getContainer()->get('ezpublish.api.repository');   $dbHandler = $this->getContainer()->get('ezpublish.connection'); $conn = $dbHandler->getConnection(); //$conn->setNestTransactionsWithSavepoints(true);   $repo->beginTransaction(); $repo->commit(); $repo->beginTransaction(); $repo->rollback(); echo "BCBR: OK\n";   $repo->beginTransaction(); $repo->rollback(); $repo->beginTransaction(); $repo->commit(); echo "BRBC: OK\n";   $repo->beginTransaction(); $repo->beginTransaction(); $repo->commit(); $repo->commit(); echo "BBCC: OK\n";   $repo->beginTransaction(); $repo->beginTransaction(); $repo->commit(); $repo->rollback(); echo "BBCR: OK\n";   $repo->beginTransaction(); $repo->beginTransaction(); $repo->rollback(); $repo->rollback(); echo "BBRR: OK\n";   // will throw unless partial rollbacks (savepoints) are enabled $repo->beginTransaction(); $repo->beginTransaction(); $repo->rollback(); $repo->beginTransaction(); $repo->commit(); $repo->commit(); echo "BBRBCC: OK\n";   // will throw unless partial rollbacks (savepoints) are enabled $repo->beginTransaction(); $repo->beginTransaction(); $repo->rollback(); $repo->commit(); echo "BBRC: OK\n"; Long story short: nested transactions are supported we could probably recommend that users set the nested_transactions to on by default in their doctrine configuration
        Hide
        Gaetano Giunta added a comment - - edited

        Further findings: the following code is problematic:

        $this->repository->beginTransaction();
        try {
            ... do stuff ...
            $this->repository->commit();
        } catch (\Exception $e) {
            $this->repository->rollback();
        }
        

        The problem is it if there is an exception in the repository commit-queue events, the repository commit() function will only throw after having called a database commit. Then the user code above will call rollback(), and the db will answer that there is no more transaction active, thus raising one further exception!

        Imho it should be easier for user-code to handle this case. At the bare minimum the repository->commit() call should return 2 separate exceptions for db-failure and event-failure (or an exception which makes it easy to distinguish the 2... at the moment, all we get is a RuntimeException with another wrapped RintimeException)

        Show
        Gaetano Giunta added a comment - - edited Further findings: the following code is problematic: $this->repository->beginTransaction(); try { ... do stuff ... $this->repository->commit(); } catch (\Exception $e) { $this->repository->rollback(); } The problem is it if there is an exception in the repository commit-queue events, the repository commit() function will only throw after having called a database commit. Then the user code above will call rollback(), and the db will answer that there is no more transaction active, thus raising one further exception! Imho it should be easier for user-code to handle this case. At the bare minimum the repository->commit() call should return 2 separate exceptions for db-failure and event-failure (or an exception which makes it easy to distinguish the 2... at the moment, all we get is a RuntimeException with another wrapped RintimeException)
        Show
        André Rømcke added a comment - Merged in https://github.com/ezsystems/ezpublish-kernel/commit/664532b4c9b196f94ed7252d45d2f5c500bc6ef6
        Hide
        André Rømcke added a comment -

        Gaetano Giunta Open a new issue if there is more in here

        Show
        André Rømcke added a comment - Gaetano Giunta Open a new issue if there is more in here

          People

          • Assignee:
            Unassigned
            Reporter:
            Gaetano Giunta
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: