Merge pull request #8323 from kenjis/fix-redis-session

fix: [Session] Redis session race condition

Author: kenjis

system/Session/Handlers/RedisHandler.php

@@ -115,6 +115,8 @@ protected function setSavePath(): void
      *
      * @param string $path The path where to store/retrieve the session
      * @param string $name The session name
+     *
+     * @throws RedisException
      */
     public function open($path, $name): bool
     {
@@ -124,12 +126,20 @@ public function open($path, $name): bool
 
         $redis = new Redis();
 
-        if (! $redis->connect($this->savePath['protocol'] . '://' . $this->savePath['host'], ($this->savePath['host'][0] === '/' ? 0 : $this->savePath['port']), $this->savePath['timeout'])) {
+        if (
+            ! $redis->connect(
+                $this->savePath['protocol'] . '://' . $this->savePath['host'],
+                ($this->savePath['host'][0] === '/' ? 0 : $this->savePath['port']),
+                $this->savePath['timeout']
+            )
+        ) {
             $this->logger->error('Session: Unable to connect to Redis with the configured settings.');
         } elseif (isset($this->savePath['password']) && ! $redis->auth($this->savePath['password'])) {
             $this->logger->error('Session: Unable to authenticate to Redis instance.');
         } elseif (isset($this->savePath['database']) && ! $redis->select($this->savePath['database'])) {
-            $this->logger->error('Session: Unable to select Redis database with index ' . $this->savePath['database']);
+            $this->logger->error(
+                'Session: Unable to select Redis database with index ' . $this->savePath['database']
+            );
         } else {
             $this->redis = $redis;
 
@@ -146,6 +156,8 @@ public function open($path, $name): bool
      *
      * @return false|string Returns an encoded string of the read data.
      *                      If nothing was read, it must return false.
+     *
+     * @throws RedisException
      */
     #[ReturnTypeWillChange]
     public function read($id)
@@ -168,14 +180,16 @@ public function read($id)
             return $data;
         }
 
-        return '';
+        return false;
     }
 
     /**
      * Writes the session data to the session storage.
      *
      * @param string $id   The session ID
      * @param string $data The encoded session data
+     *
+     * @throws RedisException
      */
     public function write($id, $data): bool
     {
@@ -222,8 +236,8 @@ public function close(): bool
                 $pingReply = $this->redis->ping();
 
                 if (($pingReply === true) || ($pingReply === '+PONG')) {
-                    if (isset($this->lockKey)) {
-                        $this->releaseLock();
+                    if (isset($this->lockKey) && ! $this->releaseLock()) {
+                        return false;
                     }
 
                     if (! $this->redis->close()) {
@@ -246,12 +260,16 @@ public function close(): bool
      * Destroys a session
      *
      * @param string $id The session ID being destroyed
+     *
+     * @throws RedisException
      */
     public function destroy($id): bool
     {
         if (isset($this->redis, $this->lockKey)) {
             if (($result = $this->redis->del($this->keyPrefix . $id)) !== 1) {
-                $this->logger->debug('Session: Redis::del() expected to return 1, got ' . var_export($result, true) . ' instead.');
+                $this->logger->debug(
+                    'Session: Redis::del() expected to return 1, got ' . var_export($result, true) . ' instead.'
+                );
             }
 
             return $this->destroyCookie();
@@ -278,6 +296,8 @@ public function gc($max_lifetime)
      * Acquires an emulated lock.
      *
      * @param string $sessionID Session ID
+     *
+     * @throws RedisException
      */
     protected function lockSession(string $sessionID): bool
     {
@@ -287,48 +307,49 @@ protected function lockSession(string $sessionID): bool
         // so we need to check here if the lock key is for the
         // correct session ID.
         if ($this->lockKey === $lockKey) {
+            // If there is the lock, make the ttl longer.
             return $this->redis->expire($this->lockKey, 300);
         }
 
         $attempt = 0;
 
         do {
-            $ttl = $this->redis->ttl($lockKey);
-            assert(is_int($ttl));
+            $result = $this->redis->set(
+                $lockKey,
+                (string) Time::now()->getTimestamp(),
+                // NX -- Only set the key if it does not already exist.
+                // EX seconds -- Set the specified expire time, in seconds.
+                ['nx', 'ex' => 300]
+            );
 
-            if ($ttl > 0) {
-                sleep(1);
+            if (! $result) {
+                usleep(100000);
 
                 continue;
             }
 
-            if (! $this->redis->setex($lockKey, 300, (string) Time::now()->getTimestamp())) {
-                $this->logger->error('Session: Error while trying to obtain lock for ' . $this->keyPrefix . $sessionID);
-
-                return false;
-            }
-
             $this->lockKey = $lockKey;
             break;
-        } while (++$attempt < 30);
+        } while (++$attempt < 300);
 
-        if ($attempt === 30) {
-            log_message('error', 'Session: Unable to obtain lock for ' . $this->keyPrefix . $sessionID . ' after 30 attempts, aborting.');
+        if ($attempt === 300) {
+            $this->logger->error(
+                'Session: Unable to obtain lock for ' . $this->keyPrefix . $sessionID
+                . ' after 300 attempts, aborting.'
+            );
 
             return false;
         }
 
-        if ($ttl === -1) {
-            log_message('debug', 'Session: Lock for ' . $this->keyPrefix . $sessionID . ' had no TTL, overriding.');
-        }
-
         $this->lock = true;
 
         return true;
     }
 
     /**
      * Releases a previously acquired lock
+     *
+     * @throws RedisException
      */
     protected function releaseLock(): bool
     {