Doctrine 1
  1. Doctrine 1
  2. DC-526

Savepoint-rollback ignored in nested transaction context.

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.2.1
    • Fix Version/s: None
    • Component/s: Transactions
    • Labels:
      None

      Description

      The example from the documentation on savepoints:

       
      try {
          $conn->beginTransaction();
          // do some operations here 
      
          // creates a new savepoint called mysavepoint
          $conn->beginTransaction('mysavepoint');
          try {
              // do some operations here
      
              $conn->commit('mysavepoint');
          } catch(Exception $e) {
              $conn->rollback('mysavepoint'); # (a)
          }
          $conn->commit();
      } catch(Exception $e) {
          $conn->rollback();
      }
      

      What seems to be a bug to me: (a) doesn't actually do a savepoint-rollback if reached, but rather returns false as soon as the nesting level is seen to be > 1.

        Activity

        Hide
        Jonathan H. Wage added a comment -

        What rdbms are you using? Does it support this feature?

        Show
        Jonathan H. Wage added a comment - What rdbms are you using? Does it support this feature?
        Hide
        Michael Nielsen added a comment -

        I use MySQL 5.0.51/InnoDB. It is supported and the doctrine-example does indeed work, if I hack the rollback-method a bit.

        316	    public function rollback($savepoint = null)
        317	    {
        318	        if ($this->_nestingLevel == 0) {
        319	            throw new Doctrine_Transaction_Exception("Rollback failed. There is no active transaction.");
        320	        }
        321	       
        322	        $this->conn->connect();
        323	
        324	        if ($this->_internalNestingLevel >= 1 && $this->_nestingLevel > 1) {
        325	            $this->_internalNestingLevel--;
        326	            $this->_nestingLevel--;
        327	            return false;
        328	        } else if ($this->_nestingLevel > 1) {
        329	            $this->_nestingLevel--;
        330	            return false;
        331	        }
        332	
        333	        $listener = $this->conn->getAttribute(Doctrine_Core::ATTR_LISTENER);
        334	
        335	        if ( ! is_null($savepoint)) {
        [...]
        

        The problem is that in a nested context, the rollback-method never does anything but a nesting-level decrement and a false-return, regardless of savepoints. So in the doctrine-example, (a) just ends up at the false-return at line 327 or 330, rather than reaching the savepoint-rollback-block starting at line 335. And so the command is never sent to MySQL.

        Show
        Michael Nielsen added a comment - I use MySQL 5.0.51/InnoDB. It is supported and the doctrine-example does indeed work, if I hack the rollback-method a bit. http://trac.doctrine-project.org/browser/tags/1.2.1/lib/Doctrine/Transaction.php 316 public function rollback($savepoint = null ) 317 { 318 if ($ this ->_nestingLevel == 0) { 319 throw new Doctrine_Transaction_Exception( "Rollback failed. There is no active transaction." ); 320 } 321 322 $ this ->conn->connect(); 323 324 if ($ this ->_internalNestingLevel >= 1 && $ this ->_nestingLevel > 1) { 325 $ this ->_internalNestingLevel--; 326 $ this ->_nestingLevel--; 327 return false ; 328 } else if ($ this ->_nestingLevel > 1) { 329 $ this ->_nestingLevel--; 330 return false ; 331 } 332 333 $listener = $ this ->conn->getAttribute(Doctrine_Core::ATTR_LISTENER); 334 335 if ( ! is_null($savepoint)) { [...] The problem is that in a nested context, the rollback-method never does anything but a nesting-level decrement and a false-return, regardless of savepoints. So in the doctrine-example, (a) just ends up at the false-return at line 327 or 330, rather than reaching the savepoint-rollback-block starting at line 335. And so the command is never sent to MySQL.
        Hide
        Jonathan H. Wage added a comment -

        Do you have a patch/diff that fixes the issue for you?

        Show
        Jonathan H. Wage added a comment - Do you have a patch/diff that fixes the issue for you?
        Hide
        Michael Nielsen added a comment -

        I just skipped the nestingLevel-section in case of a savepoint. I have not spent any time reviewing how (un)healthy that might be for the level counters and the method in general, but it fixes my current application of it.

        @@ -321,14 +321,16 @@
                 
                 $this->conn->connect();
         
        -        if ($this->_internalNestingLevel >= 1 && $this->_nestingLevel > 1) {
        -            $this->_internalNestingLevel--;
        -            $this->_nestingLevel--;
        -            return false;
        -        } else if ($this->_nestingLevel > 1) {
        -            $this->_nestingLevel--;
        -            return false;
        -        }
        +	if (is_null($savepoint)) {
        +		if ($this->_internalNestingLevel >= 1 && $this->_nestingLevel > 1) {
        +			$this->_internalNestingLevel--;
        +			$this->_nestingLevel--;
        +			return false;
        +		} else if ($this->_nestingLevel > 1) {
        +			$this->_nestingLevel--;
        +			return false;
        +		}
        +	}
         
                 $listener = $this->conn->getAttribute(Doctrine_Core::ATTR_LISTENER);
        
        Show
        Michael Nielsen added a comment - I just skipped the nestingLevel-section in case of a savepoint. I have not spent any time reviewing how (un)healthy that might be for the level counters and the method in general, but it fixes my current application of it. @@ -321,14 +321,16 @@ $ this ->conn->connect(); - if ($ this ->_internalNestingLevel >= 1 && $ this ->_nestingLevel > 1) { - $ this ->_internalNestingLevel--; - $ this ->_nestingLevel--; - return false ; - } else if ($ this ->_nestingLevel > 1) { - $ this ->_nestingLevel--; - return false ; - } + if (is_null($savepoint)) { + if ($ this ->_internalNestingLevel >= 1 && $ this ->_nestingLevel > 1) { + $ this ->_internalNestingLevel--; + $ this ->_nestingLevel--; + return false ; + } else if ($ this ->_nestingLevel > 1) { + $ this ->_nestingLevel--; + return false ; + } + } $listener = $ this ->conn->getAttribute(Doctrine_Core::ATTR_LISTENER);
        Hide
        Jonathan H. Wage added a comment -

        The change unfortunately breaks the tests.

        Show
        Jonathan H. Wage added a comment - The change unfortunately breaks the tests.
        Hide
        Hendri Kurniawan added a comment -

        Hi Jonathan,

        I can also replicate and confirm the issue. I found a workaround that will fix this issue.
        However in saying that, I'm not very familiar with Doctrine base code and may have broken something else.
        I'd be very grateful if you can have a look at the following fix.

        The patch is for Doctrine/Transaction.php

        @@ -214,9 +214,9 @@
                         }
                         $listener->postTransactionBegin($event);
                     }
        -        }
         
                 $level = ++$this->_nestingLevel;
        +        }
         
                 return $level;
             }
        @@ -244,7 +244,7 @@
                 $listener = $this->conn->getAttribute(Doctrine_Core::ATTR_LISTENER);
         
                 if ( ! is_null($savepoint)) {
        -            $this->_nestingLevel -= $this->removeSavePoints($savepoint);
        +            $this->removeSavePoints($savepoint);
         
                     $event = new Doctrine_Event($this, Doctrine_Event::SAVEPOINT_COMMIT);
         
        @@ -333,7 +333,7 @@
                 $listener = $this->conn->getAttribute(Doctrine_Core::ATTR_LISTENER);
         
                 if ( ! is_null($savepoint)) {
        -            $this->_nestingLevel -= $this->removeSavePoints($savepoint);
        +            $this->removeSavePoints($savepoint);
         
                     $event = new Doctrine_Event($this, Doctrine_Event::SAVEPOINT_ROLLBACK);
         
        @@ -507,7 +507,10 @@
              */
             public function beginInternalTransaction($savepoint = null)
             {
        +        if (is_null($savepoint)) {
                 $this->_internalNestingLevel++;
        +        }
        +
                 return $this->beginTransaction($savepoint);
             }
        

        Thanks

        Show
        Hendri Kurniawan added a comment - Hi Jonathan, I can also replicate and confirm the issue. I found a workaround that will fix this issue. However in saying that, I'm not very familiar with Doctrine base code and may have broken something else. I'd be very grateful if you can have a look at the following fix. The patch is for Doctrine/Transaction.php @@ -214,9 +214,9 @@ } $listener->postTransactionBegin($event); } - } $level = ++$ this ->_nestingLevel; + } return $level; } @@ -244,7 +244,7 @@ $listener = $ this ->conn->getAttribute(Doctrine_Core::ATTR_LISTENER); if ( ! is_null($savepoint)) { - $ this ->_nestingLevel -= $ this ->removeSavePoints($savepoint); + $ this ->removeSavePoints($savepoint); $event = new Doctrine_Event($ this , Doctrine_Event::SAVEPOINT_COMMIT); @@ -333,7 +333,7 @@ $listener = $ this ->conn->getAttribute(Doctrine_Core::ATTR_LISTENER); if ( ! is_null($savepoint)) { - $ this ->_nestingLevel -= $ this ->removeSavePoints($savepoint); + $ this ->removeSavePoints($savepoint); $event = new Doctrine_Event($ this , Doctrine_Event::SAVEPOINT_ROLLBACK); @@ -507,7 +507,10 @@ */ public function beginInternalTransaction($savepoint = null ) { + if (is_null($savepoint)) { $ this ->_internalNestingLevel++; + } + return $ this ->beginTransaction($savepoint); } Thanks

          People

          • Assignee:
            Jonathan H. Wage
            Reporter:
            Michael Nielsen
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated: