From 736a8c9d0c0cd146e843af82df690973b0a5719d Mon Sep 17 00:00:00 2001 From: "J. King" Date: Thu, 22 Nov 2018 13:30:13 -0500 Subject: [PATCH] Improved timeout handling for both SQlite and PostgreSQL --- lib/Conf.php | 8 +++-- lib/Db/PDOError.php | 3 ++ lib/Db/PostgreSQL/Driver.php | 29 +++++++++++++++++-- lib/Db/SQLite3/Driver.php | 22 ++++++++++---- .../{TestDriver.php => TestCreation.php} | 7 +++-- tests/phpunit.xml | 2 +- 6 files changed, 58 insertions(+), 13 deletions(-) rename tests/cases/Db/PostgreSQL/{TestDriver.php => TestCreation.php} (94%) diff --git a/lib/Conf.php b/lib/Conf.php index f15926c3..4572cd39 100644 --- a/lib/Conf.php +++ b/lib/Conf.php @@ -19,12 +19,16 @@ class Conf { public $dbDriver = Db\SQLite3\Driver::class; /** @var boolean Whether to attempt to automatically update the database when updated to a new version with schema changes */ public $dbAutoUpdate = true; + /** @var float Number of seconds to wait before returning a timeout error when connecting to a database (zero waits forever; not applicable to SQLite) */ + public $dbTimeoutConnect = 5.0; + /** @var float Number of seconds to wait before returning a timeout error when executing a database operation (zero waits forever; not applicable to SQLite) */ + public $dbTimeoutExec = 0.0; /** @var string|null Full path and file name of SQLite database (if using SQLite) */ public $dbSQLite3File = null; /** @var string Encryption key to use for SQLite database (if using a version of SQLite with SEE) */ public $dbSQLite3Key = ""; - /** @var integer Number of seconds for SQLite to wait before returning a timeout error when writing to the database */ - public $dbSQLite3Timeout = 60; + /** @var float Number of seconds for SQLite to wait before returning a timeout error when trying to acquire a write lock on the database (zero does not wait) */ + public $dbSQLite3Timeout = 60.0; /** @var string Host name, address, or socket path of PostgreSQL database server (if using PostgreSQL) */ public $dbPostgreSQLHost = ""; /** @var string Log-in user name for PostgreSQL database server (if using PostgreSQL) */ diff --git a/lib/Db/PDOError.php b/lib/Db/PDOError.php index 206e0224..d9ee7c86 100644 --- a/lib/Db/PDOError.php +++ b/lib/Db/PDOError.php @@ -19,6 +19,9 @@ trait PDOError { case "23000": case "23502": return [ExceptionInput::class, "constraintViolation", $err[2]]; + case "55P03": + case "57014": + return [ExceptionTimeout::class, 'general', $err[2]]; case "HY000": // engine-specific errors switch ($this->db->getAttribute(\PDO::ATTR_DRIVER_NAME)) { diff --git a/lib/Db/PostgreSQL/Driver.php b/lib/Db/PostgreSQL/Driver.php index ef09aad8..5e9f6372 100644 --- a/lib/Db/PostgreSQL/Driver.php +++ b/lib/Db/PostgreSQL/Driver.php @@ -35,6 +35,7 @@ class Driver extends \JKingWeb\Arsse\Db\AbstractDriver { $base = [ 'client_encoding' => "UTF8", 'application_name' => "arsse", + 'connect_timeout' => (string) ceil(Arsse::$conf->dbTimeoutConnect ?? 0), ]; $out = []; if ($service != "") { @@ -66,9 +67,11 @@ class Driver extends \JKingWeb\Arsse\Db\AbstractDriver { } public static function makeSetupQueries(string $schema = ""): array { + $timeout = ceil(Arsse::$conf->dbTimeoutExec * 1000); $out = [ "SET TIME ZONE UTC", - "SET DateStyle = 'ISO, MDY'" + "SET DateStyle = 'ISO, MDY'", + "SET statement_timeout = '$timeout'", ]; if (strlen($schema) > 0) { $out[] = 'SET search_path = \'"'.str_replace('"', '""', $schema).'", "$user", public\''; @@ -100,8 +103,30 @@ class Driver extends \JKingWeb\Arsse\Db\AbstractDriver { return $this->query("SELECT pg_encoding_to_char(encoding) from pg_database where datname = current_database()")->getValue() == "UTF8"; } + public function savepointCreate(bool $lock = false): int { + if (!$this->transDepth) { + $this->exec("BEGIN TRANSACTION"); + } + return parent::savepointCreate($lock); + } + + public function savepointRelease(int $index = null): bool { + $out = parent::savepointUndo($index); + if ($out && !$this->transDepth) { + $this->exec("COMMIT TRANSACTION"); + } + return $out; + } + + public function savepointUndo(int $index = null): bool { + $out = parent::savepointUndo($index); + if ($out && !$this->transDepth) { + $this->exec("ROLLBACK TRANSACTION"); + } + return $out; + } + protected function lock(): bool { - $this->exec("BEGIN TRANSACTION"); if ($this->schemaVersion()) { $this->exec("LOCK TABLE arsse_meta IN EXCLUSIVE MODE NOWAIT"); } diff --git a/lib/Db/SQLite3/Driver.php b/lib/Db/SQLite3/Driver.php index 5b94bc00..f3660488 100644 --- a/lib/Db/SQLite3/Driver.php +++ b/lib/Db/SQLite3/Driver.php @@ -27,13 +27,8 @@ class Driver extends \JKingWeb\Arsse\Db\AbstractDriver { } // if no database file is specified in the configuration, use a suitable default $dbFile = $dbFile ?? Arsse::$conf->dbSQLite3File ?? \JKingWeb\Arsse\BASE."arsse.db"; - $timeout = Arsse::$conf->dbSQLite3Timeout * 1000; try { $this->makeConnection($dbFile, Arsse::$conf->dbSQLite3Key); - // set the timeout; parameters are not allowed for pragmas, but this usage should be safe - $this->exec("PRAGMA busy_timeout = $timeout"); - // set other initial options - $this->exec("PRAGMA foreign_keys = yes"); } catch (\Throwable $e) { // if opening the database doesn't work, check various pre-conditions to find out what the problem might be $files = [ @@ -55,6 +50,11 @@ class Driver extends \JKingWeb\Arsse\Db\AbstractDriver { // otherwise the database is probably corrupt throw new Exception("fileCorrupt", $dbFile); } + // set the timeout + $timeout = (int) ceil((Arsse::$conf->dbSQLite3Timeout ?? 0) * 1000); + $this->setTimeout($timeout); + // set other initial options + $this->exec("PRAGMA foreign_keys = yes"); } public static function requirementsMet(): bool { @@ -67,6 +67,10 @@ class Driver extends \JKingWeb\Arsse\Db\AbstractDriver { $this->db->enableExceptions(true); } + protected function setTimeout(int $msec) { + $this->exec("PRAGMA busy_timeout = $msec"); + } + public function __destruct() { try { $this->db->close(); @@ -157,7 +161,13 @@ class Driver extends \JKingWeb\Arsse\Db\AbstractDriver { } protected function lock(): bool { - $this->exec("BEGIN EXCLUSIVE TRANSACTION"); + $timeout = (int) $this->query("PRAGMA busy_timeout")->getValue(); + $this->setTimeout(0); + try { + $this->exec("BEGIN EXCLUSIVE TRANSACTION"); + } finally { + $this->setTimeout($timeout); + } return true; } diff --git a/tests/cases/Db/PostgreSQL/TestDriver.php b/tests/cases/Db/PostgreSQL/TestCreation.php similarity index 94% rename from tests/cases/Db/PostgreSQL/TestDriver.php rename to tests/cases/Db/PostgreSQL/TestCreation.php index 59e113bb..1190736c 100644 --- a/tests/cases/Db/PostgreSQL/TestDriver.php +++ b/tests/cases/Db/PostgreSQL/TestCreation.php @@ -6,14 +6,17 @@ declare(strict_types=1); namespace JKingWeb\Arsse\TestCase\Db\PostgreSQL; +use JKingWeb\Arsse\Arsse; use JKingWeb\Arsse\Db\PostgreSQL\Driver; /** * @covers \JKingWeb\Arsse\Db\PostgreSQL\Driver */ -class TestDriver extends \JKingWeb\Arsse\Test\AbstractTest { +class TestCreation extends \JKingWeb\Arsse\Test\AbstractTest { /** @dataProvider provideConnectionStrings */ public function testGenerateConnectionString(bool $pdo, string $user, string $pass, string $db, string $host, int $port, string $service, string $exp) { - $postfix = "application_name='arsse' client_encoding='UTF8'"; + $this->setConf(); + $timeout = (string) ceil(Arsse::$conf->dbTimeoutConnect ?? 0); + $postfix = "application_name='arsse' client_encoding='UTF8' connect_timeout='$timeout'"; $act = Driver::makeConnectionString($pdo, $user, $pass, $db, $host, $port, $service); if ($act==$postfix) { $this->assertSame($exp, ""); diff --git a/tests/phpunit.xml b/tests/phpunit.xml index 564ced7b..67914ae7 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -56,7 +56,7 @@ cases/Db/SQLite3PDO/TestDriver.php cases/Db/SQLite3PDO/TestUpdate.php - cases/Db/PostgreSQL/TestDriver.php + cases/Db/PostgreSQL/TestCreation.php cases/Db/SQLite3/Database/TestMiscellany.php