Savepoint-rollback ignored in nested transaction context.

Details

Type: Bug
Status: Open
Priority: 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

Ascending order - Click to sort in descending order

Hide

Permalink

Jonathan H. Wage added a comment - 15/Mar/10 4:26 PM

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

Show

Jonathan H. Wage added a comment - 15/Mar/10 4:26 PM What rdbms are you using? Does it support this feature?

Hide

Permalink

Michael Nielsen added a comment - 15/Mar/10 6:15 PM

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.

Show

Michael Nielsen added a comment - 15/Mar/10 6:15 PM 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

Permalink

Jonathan H. Wage added a comment - 15/Mar/10 6:23 PM

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

Show

Jonathan H. Wage added a comment - 15/Mar/10 6:23 PM Do you have a patch/diff that fixes the issue for you?

Hide

Permalink

Michael Nielsen added a comment - 15/Mar/10 7:27 PM

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 - 15/Mar/10 7:27 PM 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

Permalink

Jonathan H. Wage added a comment - 08/Jun/10 5:09 PM

The change unfortunately breaks the tests.

Show

Jonathan H. Wage added a comment - 08/Jun/10 5:09 PM The change unfortunately breaks the tests.

Hide

Permalink

Hendri Kurniawan added a comment - 06/Aug/10 3:11 AM

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 - 06/Aug/10 3:11 AM 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

Vote (0)

Watch (1)

Dates

Created:

26/Feb/10 4:19 AM

Updated:

06/Aug/10 3:11 AM