Made out-of-order transaction handling more predictable

lib/AbstractException.php

@@ -32,6 +32,9 @@ abstract class AbstractException extends \Exception {
         "Db/Exception.paramTypeUnknown"         => 10222,
         "Db/Exception.paramTypeMissing"         => 10223,
         "Db/Exception.engineErrorGeneral"       => 10224, // this symbol may have engine-specific duplicates to accomodate engine-specific error string construction
+        "Db/Exception.unknownSavepointStatus"   => 10225,
+        "Db/ExceptionSavepoint.invalid"         => 10226,
+        "Db/ExceptionSavepoint.stale"           => 10227,
         "Db/ExceptionInput.missing"             => 10231,
         "Db/ExceptionInput.whitespace"          => 10232,
         "Db/ExceptionInput.tooLong"             => 10233,

lib/Db/AbstractDriver.php

@@ -5,6 +5,7 @@ use JKingWeb\DrUUID\UUID as UUID;
 
 abstract class AbstractDriver implements Driver {
     protected $transDepth = 0;
+    protected $transStatus = [];
 
     public function schemaVersion(): int {
         try {
@@ -18,33 +19,89 @@ abstract class AbstractDriver implements Driver {
         return new Transaction($this);
     }
     
-    public function savepointCreate(): bool {
+    public function savepointCreate(): int {
         $this->exec("SAVEPOINT arsse_".(++$this->transDepth));
-        return true;
+        $this->transStatus[$this->transDepth] = self::TR_PEND;
+        return $this->transDepth;
     }
 
-    public function savepointRelease(bool $all = false): bool {
+    public function savepointRelease(int $index = null): bool {
-        if($this->transDepth==0) return false;
+        if(is_null($index)) $index = $this->transDepth;
-        if(!$all) {
+        if(array_key_exists($index, $this->transStatus)) {
-            $this->exec("RELEASE SAVEPOINT arsse_".($this->transDepth--));
+            switch($this->transStatus[$index]) {
+                case self::TR_PEND:
+                    $this->exec("RELEASE SAVEPOINT arsse_".$index);
+                    $this->transStatus[$index] = self::TR_COMMIT;
+                    $a = $index;
+                    while(++$a && $a <= $this->transDepth) {
+                        if($this->transStatus[$a] <= self::TR_PEND) $this->transStatus[$a] = self::TR_PEND_COMMIT;
+                    }
+                    $out = true;
+                    break;
+                case self::TR_PEND_COMMIT:
+                    $this->transStatus[$index] = self::TR_COMMIT;
+                    $out = true;
+                    break;
+                case self::TR_PEND_ROLLBACK:
+                    $this->transStatus[$index] = self::TR_COMMIT;
+                    $out = false;
+                    break;
+                case self::TR_COMMIT:
+                case self::TR_ROLLBACK:
+                    throw new ExceptionSavepoint("stale", ['action' => "commit", 'index' => $index]);
+                default:
+                    throw new Exception("unknownSavepointStatus", $this->transStatus[$index]);
+            }
+            if($index==$this->transDepth) {
+                while($this->transDepth > 0 && $this->transStatus[$this->transDepth] > self::TR_PEND) {
+                    array_pop($this->transStatus);
+                    $this->transDepth--;
+                }
+            }
+            return $out;
         } else {
-            $this->exec("COMMIT TRANSACTION");
+            throw new ExceptionSavepoint("invalid", ['action' => "commit", 'index' => $index]);
-            $this->transDepth = 0;
         }
-        return true;
     }
 
-    public function savepointUndo(bool $all = false): bool {
+    public function savepointUndo(int $index = null): bool {
-        if($this->transDepth==0) return false;
+        if(is_null($index)) $index = $this->transDepth;
-        if(!$all) {
+        if(array_key_exists($index, $this->transStatus)) {
-            $this->exec("ROLLBACK TRANSACTION TO SAVEPOINT arsse_".($this->transDepth));
+            switch($this->transStatus[$index]) {
-            // rollback to savepoint does not collpase the savepoint
+                case self::TR_PEND:
-            $this->exec("RELEASE SAVEPOINT arsse_".($this->transDepth--));
+                    $this->exec("ROLLBACK TRANSACTION TO SAVEPOINT arsse_".$index);
+                    $this->exec("RELEASE SAVEPOINT arsse_".$index);
+                    $this->transStatus[$index] = self::TR_ROLLBACK;
+                    $a = $index;
+                    while(++$a && $a <= $this->transDepth) {
+                        if($this->transStatus[$a] <= self::TR_PEND) $this->transStatus[$a] = self::TR_PEND_ROLLBACK;
+                    }
+                    $out = true;
+                    break;
+                case self::TR_PEND_COMMIT:
+                    $this->transStatus[$index] = self::TR_ROLLBACK;
+                    $out = false;
+                    break;
+                case self::TR_PEND_ROLLBACK:
+                    $this->transStatus[$index] = self::TR_ROLLBACK;
+                    $out = true;
+                    break;
+                case self::TR_COMMIT:
+                case self::TR_ROLLBACK:
+                    throw new ExceptionSavepoint("stale", ['action' => "rollback", 'index' => $index]);
+                default:
+                    throw new Exception("unknownSavepointStatus", $this->transStatus[$index]);
+            }
+            if($index==$this->transDepth) {
+                while($this->transDepth > 0 && $this->transStatus[$this->transDepth] > self::TR_PEND) {
+                    array_pop($this->transStatus);
+                    $this->transDepth--;
+                }
+            }
+            return $out;
         } else {
-            $this->exec("ROLLBACK TRANSACTION");
+            throw new ExceptionSavepoint("invalid", ['action' => "rollback", 'index' => $index]);
-            $this->transDepth = 0;
         }
-        return true;
     }
 
     public function lock(): bool {

lib/Db/Driver.php

@@ -3,6 +3,12 @@ declare(strict_types=1);
 namespace JKingWeb\Arsse\Db;
 
 interface Driver {
+    const TR_PEND = 0;
+    const TR_COMMIT = 1;
+    const TR_ROLLBACK = 2;
+    const TR_PEND_COMMIT = -1;
+    const TR_PEND_ROLLBACK = -2;
+    
     function __construct(bool $install = false);
     // returns a human-friendly name for the driver (for display in installer, for example)
     static function driverName(): string;
@@ -11,11 +17,11 @@ interface Driver {
     // return a Transaction object
     function begin(): Transaction;
     // manually begin a real or synthetic transactions, with real or synthetic nesting
-    function savepointCreate(): bool;
+    function savepointCreate(): int;
     // manually commit either the latest or all pending nested transactions
-    function savepointRelease(bool $all = false): bool;
+    function savepointRelease(int $index = null): bool;
     // manually rollback either the latest or all pending nested transactions
-    function savepointUndo(bool $all = false): bool;
+    function savepointUndo(int $index = null): bool;
     // attempt to advise other processes that they should not attempt to access the database; used during live upgrades
     function lock(): bool;
     function unlock(): bool;

lib/Db/ExceptionSavepoint.php

@@ -0,0 +1,6 @@
+<?php
+declare(strict_types=1);
+namespace JKingWeb\Arsse\Db;
+
+class ExceptionSavepoint extends \JKingWeb\Arsse\AbstractException {
+}

lib/Db/SQLite3/Driver.php

@@ -69,7 +69,7 @@ class Driver extends \JKingWeb\Arsse\Db\AbstractDriver {
         $sep = \DIRECTORY_SEPARATOR;
         $path = Data::$conf->dbSchemaBase.$sep."SQLite3".$sep;
         $this->lock();
-        $this->savepointCreate();
+        $tr = $this->savepointCreate();
         for($a = $ver; $a < $to; $a++) {
             $this->savepointCreate();
             try {
@@ -87,9 +87,9 @@ class Driver extends \JKingWeb\Arsse\Db\AbstractDriver {
             } catch(\Throwable $e) {
                 // undo any partial changes from the failed update
                 $this->savepointUndo();
-                $this->unlock();
                 // commit any successful updates if updating by more than one version
-                $this->savepointRelease(true);
+                $this->unlock();
+                $this->savepointRelease();
                 // throw the error received
                 throw $e;
             }

lib/Db/Transaction.php

@@ -3,11 +3,12 @@ declare(strict_types=1);
 namespace JKingWeb\Arsse\Db;
 
 class Transaction {
+    protected $index;
     protected $pending = false;
     protected $drv;
 
     function __construct(Driver $drv) {
-        $drv->savepointCreate();
+        $this->index = $drv->savepointCreate();
         $this->drv = $drv;
         $this->pending = true;
     }
@@ -15,7 +16,7 @@ class Transaction {
     function __destruct() {
         if($this->pending) {
             try {
-                $this->drv->savepointUndo();
+                $this->drv->savepointUndo($this->index);
             } catch(\Throwable $e) {
                 // do nothing
             }
@@ -23,22 +24,22 @@ class Transaction {
     }
 
     function commit(): bool {
-        if($this->pending) {
+        $out = $this->drv->savepointRelease($this->index);
-            $this->drv->savepointRelease();
+        $this->pending = false;
-            $this->pending = false;
+        return $out;
-            return true;
-        } else {
-            return false;
-        }
     }
 
     function rollback(): bool {
-        if($this->pending) {
+        $out = $this->drv->savepointUndo($this->index);
-            $this->drv->savepointUndo();
+        $this->pending = false;
-            $this->pending = false;
+        return $out;
-            return true;
+    }
-        } else {
+
-            return false;
+    function getIndex(): int {
-        }
+        return $this->index;
+    }
+
+    function isPending(): bool {
+        return $this->pending;
     }
 }

locale/en.php

@@ -57,6 +57,7 @@ return [
             other {Automatic updating of the {driver_name} database failed because its version, {current}, is newer than the requested version, {target}}
         }',
     'Exception.JKingWeb/Arsse/Db/Exception.engineErrorGeneral'       => '{0}',
+    'Exception.JKingWeb/Arsse/Db/Exception.unknownSavepointStatus'   => 'Savepoint status code {0} not implemented',
     'Exception.JKingWeb/Arsse/Db/ExceptionInput.missing'             => 'Required field "{field}" missing while performing action "{action}"',
     'Exception.JKingWeb/Arsse/Db/ExceptionInput.whitespace'          => 'Required field "{field}" of action "{action}" may not contain only whitespace',
     'Exception.JKingWeb/Arsse/Db/ExceptionInput.tooLong'             => 'Required field "{field}" of action "{action}" has a maximum length of {max}',
@@ -66,6 +67,8 @@ return [
     'Exception.JKingWeb/Arsse/Db/ExceptionInput.constraintViolation' => '{0}',
     'Exception.JKingWeb/Arsse/Db/ExceptionInput.typeViolation'       => '{0}',
     'Exception.JKingWeb/Arsse/Db/ExceptionTimeout.general'           => '{0}',
+    'Exception.JKingWeb/Arsse/Db/ExceptionSavepoint.invalid'         => 'Tried to {action} invalid savepoint {index}',
+    'Exception.JKingWeb/Arsse/Db/ExceptionSavepoint.stale'           => 'Tried to {action} stale savepoint {index}',
     'Exception.JKingWeb/Arsse/User/Exception.alreadyExists'          => 'Could not perform action "{action}" because the user {user} already exists',
     'Exception.JKingWeb/Arsse/User/Exception.doesNotExist'           => 'Could not perform action "{action}" because the user {user} does not exist',
     'Exception.JKingWeb/Arsse/User/Exception.authMissing'            => 'Please log in to proceed',

tests/Db/SQLite3/TestDbDriverSQLite3.php

@@ -8,6 +8,7 @@ class TestDbDriverSQLite3 extends \PHPUnit\Framework\TestCase {
 
     protected $data;
     protected $drv;
+    protected $ch;
 
     function setUp() {
         $this->clearData();
@@ -16,10 +17,12 @@ class TestDbDriverSQLite3 extends \PHPUnit\Framework\TestCase {
         $conf->dbSQLite3File = tempnam(sys_get_temp_dir(), 'ook');
         Data::$conf = $conf;
         $this->drv = new Db\SQLite3\Driver(true);
+        $this->ch = new \SQLite3(Data::$conf->dbSQLite3File);
     }
 
     function tearDown() {
         unset($this->drv);
+        unset($this->ch);
         unlink(Data::$conf->dbSQLite3File);
         $this->clearData();
     }
@@ -40,13 +43,11 @@ class TestDbDriverSQLite3 extends \PHPUnit\Framework\TestCase {
 
     function testExecMultipleStatements() {
         $this->assertTrue($this->drv->exec("CREATE TABLE test(id integer primary key); INSERT INTO test(id) values(2112)"));
-        $ch = new \SQLite3(Data::$conf->dbSQLite3File);
+        $this->assertEquals(2112, $this->ch->querySingle("SELECT id from test"));
-        $this->assertEquals(2112, $ch->querySingle("SELECT id from test"));
     }
 
     function testExecTimeout() {
-        $ch = new \SQLite3(Data::$conf->dbSQLite3File);
+        $this->ch->exec("BEGIN EXCLUSIVE TRANSACTION");
-        $ch->exec("BEGIN EXCLUSIVE TRANSACTION");
         $this->assertException("general", "Db", "ExceptionTimeout");
         $this->drv->exec("CREATE TABLE test(id integer primary key)");
     }
@@ -73,8 +74,7 @@ class TestDbDriverSQLite3 extends \PHPUnit\Framework\TestCase {
     }
 
     function testQueryTimeout() {
-        $ch = new \SQLite3(Data::$conf->dbSQLite3File);
+        $this->ch->exec("BEGIN EXCLUSIVE TRANSACTION");
-        $ch->exec("BEGIN EXCLUSIVE TRANSACTION");
         $this->assertException("general", "Db", "ExceptionTimeout");
         $this->drv->query("CREATE TABLE test(id integer primary key)");
     }
@@ -101,122 +101,189 @@ class TestDbDriverSQLite3 extends \PHPUnit\Framework\TestCase {
         $s = $this->drv->prepare("This is an invalid query", "int", "int");
     }
 
-    function testBeginTransaction() {
+    function testCreateASavepoint() {
+        $this->assertEquals(1, $this->drv->savepointCreate());
+        $this->assertEquals(2, $this->drv->savepointCreate());
+        $this->assertEquals(3, $this->drv->savepointCreate());
+    }
+
+    function testReleaseASavepoint() {
+        $this->assertEquals(1, $this->drv->savepointCreate());
+        $this->assertEquals(true, $this->drv->savepointRelease());
+        $this->assertException("invalid", "Db", "ExceptionSavepoint");
+        $this->drv->savepointRelease();
+    }
+
+    function testUndoASavepoint() {
+        $this->assertEquals(1, $this->drv->savepointCreate());
+        $this->assertEquals(true, $this->drv->savepointUndo());
+        $this->assertException("invalid", "Db", "ExceptionSavepoint");
+        $this->drv->savepointUndo();
+    }
+
+    function testManipulateSavepoints() {
+        $this->assertEquals(1, $this->drv->savepointCreate());
+        $this->assertEquals(2, $this->drv->savepointCreate());
+        $this->assertEquals(3, $this->drv->savepointCreate());
+        $this->assertEquals(4, $this->drv->savepointCreate());
+        $this->assertEquals(5, $this->drv->savepointCreate());
+        $this->assertEquals(true, $this->drv->savepointUndo(3));
+        $this->assertEquals(false, $this->drv->savepointRelease(4));
+        $this->assertEquals(6, $this->drv->savepointCreate());
+        $this->assertEquals(false, $this->drv->savepointRelease(5));
+        $this->assertEquals(true, $this->drv->savepointRelease(6));
+        $this->assertEquals(3, $this->drv->savepointCreate());
+        $this->assertEquals(true, $this->drv->savepointRelease(2));
+        $this->assertException("stale", "Db", "ExceptionSavepoint");
+        $this->drv->savepointRelease(2);
+    }
+
+    function testBeginATransaction() {
         $select = "SELECT count(*) FROM test";
         $insert = "INSERT INTO test(id) values(null)";
-        $ch = new \SQLite3(Data::$conf->dbSQLite3File);
         $this->drv->exec("CREATE TABLE test(id integer primary key)");
         $tr = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(1, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
         $this->drv->query($insert);
         $this->assertEquals(2, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
     }
 
-    function testCommitTransaction() {
+    function testCommitATransaction() {
         $select = "SELECT count(*) FROM test";
         $insert = "INSERT INTO test(id) values(null)";
-        $ch = new \SQLite3(Data::$conf->dbSQLite3File);
         $this->drv->exec("CREATE TABLE test(id integer primary key)");
         $tr = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(1, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
         $tr->commit();
         $this->assertEquals(1, $this->drv->query($select)->getValue());
-        $this->assertEquals(1, $ch->querySingle($select));
+        $this->assertEquals(1, $this->ch->querySingle($select));
     }
 
-    function testRollbackTransaction() {
+    function testRollbackATransaction() {
         $select = "SELECT count(*) FROM test";
         $insert = "INSERT INTO test(id) values(null)";
-        $ch = new \SQLite3(Data::$conf->dbSQLite3File);
         $this->drv->exec("CREATE TABLE test(id integer primary key)");
         $tr = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(1, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
         $tr->rollback();
         $this->assertEquals(0, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
     }
 
     function testBeginChainedTransactions() {
         $select = "SELECT count(*) FROM test";
         $insert = "INSERT INTO test(id) values(null)";
-        $ch = new \SQLite3(Data::$conf->dbSQLite3File);
         $this->drv->exec("CREATE TABLE test(id integer primary key)");
         $tr1 = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(1, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
         $tr2 = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(2, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
     }
 
     function testCommitChainedTransactions() {
         $select = "SELECT count(*) FROM test";
         $insert = "INSERT INTO test(id) values(null)";
-        $ch = new \SQLite3(Data::$conf->dbSQLite3File);
         $this->drv->exec("CREATE TABLE test(id integer primary key)");
         $tr1 = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(1, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
         $tr2 = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(2, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
         $tr2->commit();
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
         $tr1->commit();
-        $this->assertEquals(2, $ch->querySingle($select));
+        $this->assertEquals(2, $this->ch->querySingle($select));
+    }
+
+    function testCommitChainedTransactionsOutOfOrder() {
+        $select = "SELECT count(*) FROM test";
+        $insert = "INSERT INTO test(id) values(null)";
+        $this->drv->exec("CREATE TABLE test(id integer primary key)");
+        $tr1 = $this->drv->begin();
+        $this->drv->query($insert);
+        $this->assertEquals(1, $this->drv->query($select)->getValue());
+        $this->assertEquals(0, $this->ch->querySingle($select));
+        $tr2 = $this->drv->begin();
+        $this->drv->query($insert);
+        $this->assertEquals(2, $this->drv->query($select)->getValue());
+        $this->assertEquals(0, $this->ch->querySingle($select));
+        $tr1->commit();
+        $this->assertEquals(2, $this->ch->querySingle($select));
+        $tr2->commit();
     }
 
     function testRollbackChainedTransactions() {
         $select = "SELECT count(*) FROM test";
         $insert = "INSERT INTO test(id) values(null)";
-        $ch = new \SQLite3(Data::$conf->dbSQLite3File);
         $this->drv->exec("CREATE TABLE test(id integer primary key)");
         $tr1 = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(1, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
         $tr2 = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(2, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
         $tr2->rollback();
         $this->assertEquals(1, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
         $tr1->rollback();
         $this->assertEquals(0, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
+    }
+
+    function testRollbackChainedTransactionsOutOfOrder() {
+        $select = "SELECT count(*) FROM test";
+        $insert = "INSERT INTO test(id) values(null)";
+        $this->drv->exec("CREATE TABLE test(id integer primary key)");
+        $tr1 = $this->drv->begin();
+        $this->drv->query($insert);
+        $this->assertEquals(1, $this->drv->query($select)->getValue());
+        $this->assertEquals(0, $this->ch->querySingle($select));
+        $tr2 = $this->drv->begin();
+        $this->drv->query($insert);
+        $this->assertEquals(2, $this->drv->query($select)->getValue());
+        $this->assertEquals(0, $this->ch->querySingle($select));
+        $tr1->rollback();
+        $this->assertEquals(0, $this->drv->query($select)->getValue());
+        $this->assertEquals(0, $this->ch->querySingle($select));
+        $tr2->rollback();
+        $this->assertEquals(0, $this->drv->query($select)->getValue());
+        $this->assertEquals(0, $this->ch->querySingle($select));
     }
 
     function testPartiallyRollbackChainedTransactions() {
         $select = "SELECT count(*) FROM test";
         $insert = "INSERT INTO test(id) values(null)";
-        $ch = new \SQLite3(Data::$conf->dbSQLite3File);
         $this->drv->exec("CREATE TABLE test(id integer primary key)");
         $tr1 = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(1, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
         $tr2 = $this->drv->begin();
         $this->drv->query($insert);
         $this->assertEquals(2, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
         $tr2->rollback();
         $this->assertEquals(1, $this->drv->query($select)->getValue());
-        $this->assertEquals(0, $ch->querySingle($select));
+        $this->assertEquals(0, $this->ch->querySingle($select));
         $tr1->commit();
         $this->assertEquals(1, $this->drv->query($select)->getValue());
-        $this->assertEquals(1, $ch->querySingle($select));
+        $this->assertEquals(1, $this->ch->querySingle($select));
     }
 
     function testFetchSchemaVersion() {