MDL-66151 Performance: Session Manager modularisation

Storage of session metadata has moved into the session handler class. This allows for other classes to fully control session handling and removes the dependancy on the core sessions database table. Previously, the standard method of interaction with the session metadata was direct DB calls; this may break other plugins as there are now proper APIs available through the session manager. Co-authored-by: Darren Cocco <[email protected]> Co-authored-by: Trisha Milan <[email protected]> Co-authored-by: Andrew Nicols <[email protected]>
abgreeve · Sep 3, 2024 · e52fbd2 · e52fbd2
1 parent 072fb90
commit e52fbd2
Show file tree

Hide file tree

Showing 30 changed files with 1,400 additions and 692 deletions.
diff --git a/.upgradenotes/MDL-66151-2024090301594788.yml b/.upgradenotes/MDL-66151-2024090301594788.yml
@@ -0,0 +1,15 @@
+issueNumber: MDL-66151
+notes:
+  core_role:
+    - message: |
+        Move all session management to the \core\session\manager class.
+        This removes the dependancy to use the "sessions" table.
+        Session management plugins (like redis) now need to inherit
+        the base \core\session\handler class which implements
+        SessionHandlerInterface and override methods as required.
+        The following methods in \core\session\manager have been deprecated:
+        * kill_all_sessions use destroy_all instead
+        * kill_session use destroy instead
+        * kill_sessions_for_auth_plugin use destroy_by_auth_plugin instead
+        * kill_user_sessions use destroy_user_sessions instead
+      type: improved
diff --git a/admin/tool/mfa/factor/email/email.php b/admin/tool/mfa/factor/email/email.php
@@ -79,7 +79,7 @@
         $DB->set_field('tool_mfa', 'revoked', 1, ['userid' => $user->id, 'factor' => 'email']);
 
         // Remotely logout all sessions for user.
-        $manager = \core\session\manager::kill_user_sessions($instance->userid);
+        \core\session\manager::destroy_user_sessions($instance->userid);
 
         // Log event.
         $ip = $instance->createdfromip;

diff --git a/admin/tool/uploaduser/classes/process.php b/admin/tool/uploaduser/classes/process.php
@@ -931,7 +931,7 @@ public function process_line(array $line) {
             }
 
             if ($dologout) {
-                \core\session\manager::kill_user_sessions($existinguser->id);
+                \core\session\manager::destroy_user_sessions($existinguser->id);
             }
 
         } else {

diff --git a/admin/user.php b/admin/user.php
@@ -131,7 +131,7 @@
             if (!is_siteadmin($user) and $USER->id != $user->id and $user->suspended != 1) {
                 $user->suspended = 1;
                 // Force logout.
-                \core\session\manager::kill_user_sessions($user->id);
+                \core\session\manager::destroy_user_sessions($user->id);
                 user_update_user($user, false);
             }
         }

diff --git a/auth/ldap/auth.php b/auth/ldap/auth.php
@@ -839,7 +839,7 @@ public function sync_users_update_callback(?callable $updatecallback = null): bo
                     $updateuser->suspended = 1;
                     user_update_user($updateuser, false);
                     echo "\t"; print_string('auth_dbsuspenduser', 'auth_db', array('name'=>$user->username, 'id'=>$user->id)); echo "\n";
-                    \core\session\manager::kill_user_sessions($user->id);
+                    \core\session\manager::destroy_user_sessions($user->id);
                 }
             } else {
                 print_string('nouserentriestoremove', 'auth_ldap');

diff --git a/auth/mnet/auth.php b/auth/mnet/auth.php
@@ -868,7 +868,7 @@ function kill_children($username, $useragent) {
                                  array('useragent'=>$useragent, 'userid'=>$userid));
 
         if (isset($remoteclient) && isset($remoteclient->id)) {
-            \core\session\manager::kill_user_sessions($userid);
+            \core\session\manager::destroy_user_sessions($userid);
         }
         return $returnstring;
     }
@@ -888,7 +888,7 @@ function kill_child($username, $useragent) {
         $session = $DB->get_record('mnet_session', array('username'=>$username, 'mnethostid'=>$remoteclient->id, 'useragent'=>$useragent));
         $DB->delete_records('mnet_session', array('username'=>$username, 'mnethostid'=>$remoteclient->id, 'useragent'=>$useragent));
         if (false != $session) {
-            \core\session\manager::kill_session($session->session_id);
+            \core\session\manager::destroy($session->session_id);
             return true;
         }
         return false;
@@ -905,7 +905,7 @@ function end_local_sessions(&$sessionArray) {
         global $CFG;
         if (is_array($sessionArray)) {
             while($session = array_pop($sessionArray)) {
-                \core\session\manager::kill_session($session->session_id);
+                \core\session\manager::destroy($session->session_id);
             }
             return true;
         }

diff --git a/auth/shibboleth/classes/helper.php b/auth/shibboleth/classes/helper.php
@@ -98,7 +98,7 @@ public static function logout_db_session($spsessionid) {
                 // If there is a match, kill the session.
                 if ($usersession['SESSION']->shibboleth_session_id == trim($spsessionid)) {
                     // Delete this user's sessions.
-                    \core\session\manager::kill_user_sessions($session->userid);
+                    \core\session\manager::destroy_user_sessions($session->userid);
                 }
             }
         }

diff --git a/cache/stores/redis/lib.php b/cache/stores/redis/lib.php
@@ -604,7 +604,8 @@ public function has_all(array $keys) {
      * @return bool True if the lock was acquired, false if it was not.
      */
     public function acquire_lock($key, $ownerid) {
-        $timelimit = time() + $this->lockwait;
+        $clock = \core\di::get(\core\clock::class);
+        $timelimit = $clock->time() + $this->lockwait;
         do {
             // If the key doesn't already exist, grab it and return true.
             if ($this->redis->setnx($key, $ownerid)) {
@@ -620,7 +621,7 @@ public function acquire_lock($key, $ownerid) {
             }
             // Wait 1 second then retry.
             sleep(1);
-        } while (time() < $timelimit);
+        } while ($clock->time() < $timelimit);
         return false;
     }
 

diff --git a/lib/behat/classes/behat_session_trait.php b/lib/behat/classes/behat_session_trait.php
@@ -1163,11 +1163,11 @@ protected function get_session_user() {
         if (empty($sid)) {
             throw new coding_exception('failed to get moodle session');
         }
-        $userid = $DB->get_field('sessions', 'userid', ['sid' => $sid]);
-        if (empty($userid)) {
-            throw new coding_exception('failed to get user from seession id '.$sid);
+        $session = \core\session\manager::get_session_by_sid($sid);
+        if (empty($session->userid)) {
+            throw new coding_exception('failed to get user from session id: '.$sid);
         }
-        return $DB->get_record('user', ['id' => $userid]);
+        return $DB->get_record('user', ['id' => $session->userid]);
     }
 
     /**

diff --git a/lib/classes/plugininfo/auth.php b/lib/classes/plugininfo/auth.php
@@ -87,6 +87,7 @@ public static function enable_plugin(string $pluginname, int $enabled): bool {
             $new = implode(',', array_flip($plugins));
             add_to_config_log('auth', $CFG->auth, $new, 'core');
             set_config('auth', $new);
+            \core\session\manager::destroy_by_auth_plugin($pluginname);
             // Remove stale sessions.
             \core\session\manager::gc();
             // Reset caches.
@@ -160,6 +161,7 @@ public function uninstall_cleanup() {
             $value = implode(',', $auths);
             add_to_config_log('auth', $CFG->auth, $value, 'core');
             set_config('auth', $value);
+            \core\session\manager::destroy_by_auth_plugin($this->name);
         }
 
         if (!empty($CFG->registerauth) and $CFG->registerauth === $this->name) {

diff --git a/lib/classes/session/database.php b/lib/classes/session/database.php
@@ -14,14 +14,6 @@
 // You should have received a copy of the GNU General Public License
 // along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
 
-/**
- * Database based session handler.
- *
- * @package    core
- * @copyright  2013 Petr Skoda {@link http://skodak.org}
- * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
- */
-
 namespace core\session;
 
 use SessionHandlerInterface;
@@ -34,7 +26,8 @@
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 class database extends handler implements SessionHandlerInterface {
-    /** @var \stdClass $record session record */
+
+    /** @var int $record session record */
     protected $recordid = null;
 
     /** @var \moodle_database $database session database */
@@ -62,9 +55,7 @@ public function __construct() {
         }
     }
 
-    /**
-     * Init session handler.
-     */
+    #[\Override]
     public function init() {
         if (!$this->database->session_lock_supported()) {
             throw new exception('sessionhandlerproblem', 'error', '', null, 'Database does not support session locking');
@@ -76,35 +67,36 @@ public function init() {
         }
     }
 
-    /**
-     * Check the backend contains data for this session id.
-     *
-     * Note: this is intended to be called from manager::session_exists() only.
-     *
-     * @param string $sid
-     * @return bool true if session found.
-     */
+    #[\Override]
     public function session_exists($sid) {
         // It was already checked in the calling code that the record in sessions table exists.
         return true;
     }
 
-    /**
-     * Kill all active sessions, the core sessions table is
-     * purged afterwards.
-     */
-    public function kill_all_sessions() {
-        // Nothing to do, the sessions table is cleared from core.
-        return;
-    }
+    #[\Override]
+    public function destroy(string $id): bool {
+        if (!$session = $this->get_session_by_sid($id)) {
+            if ($id == session_id()) {
+                $this->recordid = null;
+                $this->lasthash = null;
+            }
+            return true;
+        }
 
-    /**
-     * Kill one session, the session record is removed afterwards.
-     * @param string $sid
-     */
-    public function kill_session($sid) {
-        // Nothing to do, the sessions table is purged afterwards.
-        return;
+        if ($this->recordid && ($session->id == $this->recordid)) {
+            try {
+                $this->database->release_session_lock($this->recordid);
+            } catch (\Exception $ex) {
+                // Log and ignore any problems.
+                mtrace('Failed to release session lock: '.$ex->getMessage());
+            }
+            $this->recordid = null;
+            $this->lasthash = null;
+        }
+
+        $this->database->delete_records('sessions', ['id' => $session->id]);
+
+        return true;
     }
 
     /**
@@ -151,7 +143,7 @@ public function close(): bool {
      */
     public function read(string $sid): string|false {
         try {
-            if (!$record = $this->database->get_record('sessions', array('sid'=>$sid), 'id')) {
+            if (!$record = $this->get_session_by_sid($sid)) {
                 // Let's cheat and skip locking if this is the first access,
                 // do not create the record here, let the manager do it after session init.
                 $this->failed = false;
@@ -252,56 +244,4 @@ public function write(string $id, string $data): bool {
         return true;
     }
 
-    /**
-     * Destroy session handler.
-     *
-     * {@see http://php.net/manual/en/function.session-set-save-handler.php}
-     *
-     * @param string $id
-     * @return bool success
-     */
-    public function destroy(string $id): bool {
-        if (!$session = $this->database->get_record('sessions', ['sid' => $id], 'id, sid')) {
-            if ($id == session_id()) {
-                $this->recordid = null;
-                $this->lasthash = null;
-            }
-            return true;
-        }
-
-        if ($this->recordid && ($session->id == $this->recordid)) {
-            try {
-                $this->database->release_session_lock($this->recordid);
-            } catch (\Exception $ex) {
-                // Ignore problems.
-            }
-            $this->recordid = null;
-            $this->lasthash = null;
-        }
-
-        $this->database->delete_records('sessions', ['id' => $session->id]);
-
-        return true;
-    }
-
-    /**
-     * GC session handler.
-     *
-     * {@see http://php.net/manual/en/function.session-set-save-handler.php}
-     *
-     * @param int $max_lifetime moodle uses special timeout rules
-     * @return bool success
-     */
-    // phpcs:ignore moodle.NamingConventions.ValidVariableName.VariableNameUnderscore
-    public function gc(int $max_lifetime): int|false {
-        // This should do something only if cron is not running properly...
-        if (!$stalelifetime = ini_get('session.gc_maxlifetime')) {
-            return false;
-        }
-        $params = ['purgebefore' => (time() - $stalelifetime)];
-        $count = $this->database->count_records_select('sessions', 'userid = 0 AND timemodified < :purgebefore', $params);
-        $this->database->delete_records_select('sessions', 'userid = 0 AND timemodified < :purgebefore', $params);
-
-        return $count;
-    }
 }
diff --git a/lib/classes/session/file.php b/lib/classes/session/file.php
@@ -14,18 +14,8 @@
 // You should have received a copy of the GNU General Public License
 // along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
 
-/**
- * File based session handler.
- *
- * @package    core
- * @copyright  2013 Petr Skoda {@link http://skodak.org}
- * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
- */
-
 namespace core\session;
 
-defined('MOODLE_INTERNAL') || die();
-
 /**
  * File based session handler.
  *
@@ -34,6 +24,7 @@
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  */
 class file extends handler {
+
     /** @var string session dir */
     protected $sessiondir;
 
@@ -50,9 +41,7 @@ public function __construct() {
         }
     }
 
-    /**
-     * Init session handler.
-     */
+    #[\Override]
     public function init() {
         if (preg_match('/^[0-9]+;/', $this->sessiondir)) {
             throw new exception('sessionhandlerproblem', 'error', '', null, 'Multilevel session directories are not supported');
@@ -75,14 +64,7 @@ public function init() {
         ini_set('session.save_path', $this->sessiondir);
     }
 
-    /**
-     * Check the backend contains data for this session id.
-     *
-     * Note: this is intended to be called from manager::session_exists() only.
-     *
-     * @param string $sid
-     * @return bool true if session found.
-     */
+    #[\Override]
     public function session_exists($sid) {
         $sid = clean_param($sid, PARAM_FILE);
         if (!$sid) {
@@ -92,30 +74,28 @@ public function session_exists($sid) {
         return file_exists($sessionfile);
     }
 
-    /**
-     * Kill all active sessions, the core sessions table is
-     * purged afterwards.
-     */
-    public function kill_all_sessions() {
+    #[\Override]
+    public function destroy_all(): bool {
         if (is_dir($this->sessiondir)) {
             foreach (glob("$this->sessiondir/sess_*") as $filename) {
                 @unlink($filename);
             }
         }
+
+        return true;
     }
 
-    /**
-     * Kill one session, the session record is removed afterwards.
-     * @param string $sid
-     */
-    public function kill_session($sid) {
-        $sid = clean_param($sid, PARAM_FILE);
+    #[\Override]
+    public function destroy(string $id): bool {
+        $sid = clean_param($id, PARAM_FILE);
         if (!$sid) {
-            return;
+            return false;
         }
         $sessionfile = "$this->sessiondir/sess_$sid";
         if (file_exists($sessionfile)) {
             @unlink($sessionfile);
         }
+
+        return true;
     }
 }