refactor(db_session): retry mechanism and lock timeout to optimize se…

…ssion locking and concurrency
cypht-org · Nov 29, 2024 · aa5bad6 · aa5bad6
1 parent d945bbc
commit aa5bad6
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 50 deletions.
diff --git a/lib/session_db.php b/lib/session_db.php
@@ -28,12 +28,14 @@ class Hm_DB_Session extends Hm_PHP_Session {
     */
     private $lock_timeout = 10;
 
+    private $version = 1;
+
     /**
      * Create a new session
      * @return boolean|integer|array
      */
     public function insert_session_row() {
-        return $this->upsert('insert');
+        return $this->upsert('update');
     }
 
     /**
@@ -77,17 +79,13 @@ public function start_new($request) {
      */
     public function start_existing($key) {
         $this->session_key = $key;
-        if (!$this->acquire_lock($key)) {
-            Hm_Debug::add('DB SESSION: Failed to acquire lock');
-            return;
-        }
         $data = $this->get_session_data($key);
+
         if (is_array($data)) {
             Hm_Debug::add('LOGGED IN');
             $this->active = true;
             $this->data = $data;
         }
-        $this->release_lock($key);
     }
 
     /**
@@ -96,9 +94,12 @@ public function start_existing($key) {
      * @return mixed array results or false on failure
      */
     public function get_session_data($key) {
-        $results = Hm_DB::execute($this->dbh, 'select data from hm_user_session where hm_id=?', [$key]);
-        if (is_array($results) && array_key_exists('data', $results)) {
-            return $this->plaintext($results['data']);
+        $results = Hm_DB::execute($this->dbh, 'select data, hm_version from hm_user_session where hm_id=?', [$key]);
+        if (is_array($results)) {
+            if (array_key_exists('data', $results) && array_key_exists('hm_version', $results)) {
+                $this->version = $results['hm_version'];
+                return $this->plaintext($results['data']);
+            }
         }
         Hm_Debug::add('DB SESSION failed to read session data');
         return false;
@@ -139,11 +140,28 @@ public function close_early() {
      */
     public function upsert($type) {
         $res = false;
+
         $params = [':key' => $this->session_key, ':data' => $this->ciphertext($this->data)];
         if ($type == 'update') {
-            $res = Hm_DB::execute($this->dbh, 'update hm_user_session set data=:data where hm_id=:key', $params);
+            if ($this->version === null) {
+                Hm_Debug::add('DB SESSION: Missing hm_version for session key ' . $this->session_key);
+                return false;
+            }
+            $params[':hm_version'] = $this->version;
+            if (!$this->acquire_lock($this->session_key)) {
+                Hm_Debug::add('Failed to acquire lock on session');
+                return false;
+            }
+            $res = Hm_DB::execute($this->dbh, 'update hm_user_session set data=:data, hm_version=hm_version+1 where hm_id=:key and hm_version=:hm_version', $params);
+            if ($res === 0) {
+                Hm_Debug::add('Optimistic Locking: hm_version mismatch, session data not updated');
+                $this->release_lock($this->session_key);
+                return false;
+            }
+            $this->release_lock($this->session_key);
         } elseif ($type == 'insert') {
-            $res = Hm_DB::execute($this->dbh, 'insert into hm_user_session values(:key, :data, current_date)', $params);
+            $res = Hm_DB::execute($this->dbh, 'insert into hm_user_session (hm_id, data, hm_version, date) values(:key, :data, 1, current_date)', $params);
+            Hm_Debug::add('Session insert params: ' . json_encode($params));
         }
         if (!$res) {
             Hm_Debug::add('DB SESSION failed to write session data');
@@ -192,44 +210,67 @@ public function db_start($request) {
      * @return bool true if lock acquired, false otherwise
      */
     private function acquire_lock($key) {
-        $lock_name = 'session_lock_' . substr(hash('sha256', $key), 0, 51);        
+        $lock_name = 'session_lock_' . substr(hash('sha256', $key), 0, 51);
+
+        // Polling parameters
+        $max_attempts = 5;
+        $retry_interval = 500000;
+        $attempts = 0;
+
         $query = '';
         $params = [];
-
-        switch ($this->db_driver) {
-            case 'mysql':
-                $query = 'SELECT GET_LOCK(:lock_name, :timeout)';
-                $params = [':lock_name' => $lock_name, ':timeout' => $this->lock_timeout];
-                break;
-
-            case 'pgsql':
-                $query = 'SELECT pg_try_advisory_lock(:hash_key)';
-                $params = [':hash_key' => crc32($lock_name)];
-                break;
-
-            case 'sqlite':
-                $query = 'UPDATE hm_user_session SET lock=1 WHERE hm_id=? AND lock=0';
-                $params = [$key];
-                break;
-
-            default:
-                Hm_Debug::add('DB SESSION: Unsupported db_driver for locking: ' . $this->db_driver);
-                return false;
-        }
-
-        $result = Hm_DB::execute($this->dbh, $query, $params);
-        if ($this->db_driver == 'mysql') {
-            return isset($result['GET_LOCK(?, ?)']) && $result['GET_LOCK(?, ?)'] == 1;
-        }
-        if ($this->db_driver == 'pgsql') {
-            return isset($result['pg_try_advisory_lock']) && $result['pg_try_advisory_lock'] === true;
-        }
-
-        if ($this->db_driver == 'sqlite') {
-            return isset($result[0]) && $result[0] == 1;
+
+        while ($attempts < $max_attempts) {
+            switch ($this->db_driver) {
+                case 'mysql':
+                    $query = 'SELECT GET_LOCK(:lock_name, :timeout)';
+                    $params = [':lock_name' => $lock_name, ':timeout' => $this->lock_timeout];
+                    break;
+
+                case 'pgsql':
+                    $query = 'SELECT pg_try_advisory_lock(:hash_key)';
+                    $params = [':hash_key' => crc32($lock_name)];
+                    break;
+
+                case 'sqlite':
+                    $query = 'UPDATE hm_user_session SET lock=1 WHERE hm_id=? AND lock=0';
+                    $params = [$key];
+                    break;
+
+                default:
+                    Hm_Debug::add('DB SESSION: Unsupported db_driver for locking: ' . $this->db_driver);
+                    return false;
+            }
+
+            $result = Hm_DB::execute($this->dbh, $query, $params);
+
+            if ($this->db_driver == 'mysql') {
+                if (isset($result['GET_LOCK(?, ?)']) && $result['GET_LOCK(?, ?)'] == 1) {
+                    return true;
+                }
+            }
+            if ($this->db_driver == 'pgsql') {
+                if (isset($result['pg_try_advisory_lock']) && $result['pg_try_advisory_lock'] === true) {
+                    return true;
+                }
+            }
+            if ($this->db_driver == 'sqlite') {
+                if (isset($result[0]) && $result[0] == 1) {
+                    return true;
+                }
+            }
+
+            // Lock not acquired, so increment attempts and sleep for retry
+            $attempts++;
+
+            if ($attempts < $max_attempts) {
+                usleep($retry_interval);
+            }
         }
+
+        Hm_Debug::add('DB SESSION: Failed to acquire lock after ' . $max_attempts . ' attempts.');
         return false;
-    }
+    }    
 
     /**
      * Release a lock for the session (unified for all DB types)

diff --git a/scripts/setup_database.php b/scripts/setup_database.php
@@ -132,19 +132,22 @@ function add_missing_columns($conn, $table_name, $required_columns, $db_driver)
 $tables = [
     'hm_user_session' => [
         'mysql' => [
-            'hm_id' => 'varchar(255) PRIMARY KEY',
-            'data' => 'longblob',
-            'date' => 'timestamp',
+            'hm_id'   => 'varchar(255) PRIMARY KEY',
+            'data'    => 'longblob',
+            'hm_version' => 'INT DEFAULT 1',
+            'date'    => 'timestamp',
         ],
         'sqlite' => [
             'hm_id' => 'varchar(255) PRIMARY KEY',
             'data' => 'longblob',
             'lock' => 'INTEGER DEFAULT 0',
+            'hm_version' => 'INT DEFAULT 1',
             'date' => 'timestamp',
         ],
         'pgsql' => [
             'hm_id' => 'varchar(255) PRIMARY KEY',
             'data' => 'text',
+            'hm_version' => 'INT DEFAULT 1',
             'date' => 'timestamp',
         ],
     ],

diff --git a/tests/phpunit/data/schema.sql b/tests/phpunit/data/schema.sql
@@ -1,6 +1,6 @@
 
 CREATE TABLE IF NOT EXISTS hm_user (username varchar(255), hash varchar(255), primary key (username));
 
-CREATE TABLE IF NOT EXISTS hm_user_session (hm_id varchar(255), data longblob, date timestamp, primary key (hm_id));
+CREATE TABLE IF NOT EXISTS hm_user_session (hm_id varchar(255), data longblob, date timestamp, hm_version int default 1, primary key (hm_id));
 
 CREATE TABLE IF NOT EXISTS hm_user_settings(username varchar(255), settings longblob, primary key (username));
diff --git a/tests/phpunit/data/schema_sqlite.sql b/tests/phpunit/data/schema_sqlite.sql
@@ -0,0 +1,6 @@
+
+CREATE TABLE IF NOT EXISTS hm_user (username varchar(255), hash varchar(255), primary key (username));
+
+CREATE TABLE IF NOT EXISTS hm_user_session (hm_id varchar(255), data longblob, date timestamp, lock int default 0, hm_version int default 1, primary key (hm_id));
+
+CREATE TABLE IF NOT EXISTS hm_user_settings(username varchar(255), settings longblob, primary key (username));
diff --git a/tests/phpunit/run.sh b/tests/phpunit/run.sh
@@ -20,7 +20,7 @@ if [ "$DB" = "sqlite" ]; then
     export DB_CONNECTION_TYPE=socket
     export DB_SOCKET=${FILE}
 
-    cat ${SCRIPT_DIR}/data/schema.sql | sqlite3 ${FILE}
+    cat ${SCRIPT_DIR}/data/schema_sqlite.sql | sqlite3 ${FILE}
     cat ${SCRIPT_DIR}/data/seed.sql | sqlite3 ${FILE}
 
 elif [ "$DB" = "mysql" ]; then