fix: [Session] Redis session race condition

[kenjis]

Description
Fixes #4391
Fixes #8567

• fix Redis Session race condition
• fix that close() does not return false if releaseLock() failed
• fix that read() does not return false if lockSession() failed

Testing

• macOS 12.7.2
• PHP 8.2.16
• symfony CLI version 5.7.4
• Redis 7.2.4

--- a/app/Config/Session.php
+++ b/app/Config/Session.php
@@ -4,7 +4,6 @@ namespace Config;
 
 use CodeIgniter\Config\BaseConfig;
 use CodeIgniter\Session\Handlers\BaseHandler;
-use CodeIgniter\Session\Handlers\FileHandler;
 
 class Session extends BaseConfig
 {
@@ -21,7 +20,7 @@ class Session extends BaseConfig
      *
      * @var class-string<BaseHandler>
      */
-    public string $driver = FileHandler::class;
+    public string $driver = 'CodeIgniter\Session\Handlers\RedisHandler';
 
     /**
      * --------------------------------------------------------------------------
@@ -57,7 +56,7 @@ class Session extends BaseConfig
      *
      * IMPORTANT: You are REQUIRED to set a valid save path!
      */
-    public string $savePath = WRITEPATH . 'session';
+    public string $savePath = 'tcp://localhost:6379';
 
     /**
      * --------------------------------------------------------------------------
diff --git a/app/Controllers/Home.php b/app/Controllers/Home.php
index 5934333309..bdf121cce1 100644
--- a/app/Controllers/Home.php
+++ b/app/Controllers/Home.php
@@ -4,8 +4,16 @@ namespace App\Controllers;
 
 class Home extends BaseController
 {
-    public function index(): string
+    public function index()
     {
-        return view('welcome_message');
+        $session = session();
+
+        var_dump($_SESSION);
+
+        $count = $session->get('count') ?? 0;
+        $count++;
+        $session->set('count', $count);
+
+        var_dump($_SESSION);
     }
 }

I have tested by ab -n 1000 -c 20.

Results:

develop:
Requests per second:    43.53 [#/sec] (mean)
Error while trying to free lock: 2

This PR:
Requests per second:    46.98 [#/sec] (mean)
Error while trying to free lock: 0

develop:

$ ab -n 1000 -c 20 -C 'ci_session=j8egao4miu36k4erouc0b0a2s1aagnbr' http://localhost:8000/
This is ApacheBench, Version 2.3 <$Revision: 1903618 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Completed 600 requests
Completed 700 requests
Completed 800 requests
Completed 900 requests
Completed 1000 requests
Finished 1000 requests


Server Software:        
Server Hostname:        localhost
Server Port:            8000

Document Path:          /
Document Length:        22431 bytes

Concurrency Level:      20
Time taken for tests:   22.974 seconds
Complete requests:      1000
Failed requests:        0
Total transferred:      22709000 bytes
HTML transferred:       22431000 bytes
Requests per second:    43.53 [#/sec] (mean)
Time per request:       459.486 [ms] (mean)
Time per request:       22.974 [ms] (mean, across all concurrent requests)
Transfer rate:          965.29 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.4      0       5
Processing:    52  439 362.5    345    3445
Waiting:       50  438 362.5    345    3445
Total:         52  439 362.5    346    3446

Percentage of the requests served within a certain time (ms)
  50%    346
  66%    362
  75%    377
  80%    403
  90%    478
  95%   1322
  98%   1496
  99%   2353
 100%   3446 (longest request)

ERROR - 2024-02-22 03:59:55 --> Session: Error while trying to free lock for ci_session:ci_session:j8egao4miu36k4erouc0b0a2s1aagnbr:lock
ERROR - 2024-02-22 03:59:58 --> Session: Error while trying to free lock for ci_session:ci_session:j8egao4miu36k4erouc0b0a2s1aagnbr:lock

This PR:

$ ab -n 1000 -c 20 -C 'ci_session=j8egao4miu36k4erouc0b0a2s1aagnbr' http://localhost:8000/
This is ApacheBench, Version 2.3 <$Revision: 1903618 $>
Copyright 1996 Adam Twiss, Zeus Technology Ltd, http://www.zeustech.net/
Licensed to The Apache Software Foundation, http://www.apache.org/

Benchmarking localhost (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Completed 600 requests
Completed 700 requests
Completed 800 requests
Completed 900 requests
Completed 1000 requests
Finished 1000 requests


Server Software:        
Server Hostname:        localhost
Server Port:            8000

Document Path:          /
Document Length:        22431 bytes

Concurrency Level:      20
Time taken for tests:   21.285 seconds
Complete requests:      1000
Failed requests:        0
Total transferred:      22709000 bytes
HTML transferred:       22431000 bytes
Requests per second:    46.98 [#/sec] (mean)
Time per request:       425.692 [ms] (mean)
Time per request:       21.285 [ms] (mean, across all concurrent requests)
Transfer rate:          1041.91 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.4      0       5
Processing:    68  421 155.4    374    1967
Waiting:       66  420 155.4    374    1967
Total:         68  421 155.4    375    1967

Percentage of the requests served within a certain time (ms)
  50%    375
  66%    425
  75%    465
  80%    491
  90%    604
  95%    727
  98%    872
  99%    955
 100%   1967 (longest request)

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

[kenjis]

I did simple testing on macOS, and the results was good.
The following error that happened on develop is gone.

Session: Error while trying to free lock for ci_session:ci_session:8jo0gp3q9jejphgpdc8bjn8p09skfr7b:lock

[kenjis]

Explanation of current develop code.

--- a/system/Session/Handlers/RedisHandler.php
+++ b/system/Session/Handlers/RedisHandler.php
@@ -293,16 +293,25 @@ RedisHandler::lockSession()
         $attempt = 0;
 
         do {
+            // Gets the TTL of the lock record.
+            // When there is no lock record, if two processes query Redis at the same time,
+            // both will have `$ttl = 0`.
             $ttl = $this->redis->ttl($lockKey);
             assert(is_int($ttl));
 
             if ($ttl > 0) {
+                // If the TTL is longer than zero second, that is the lock record exists,
+                // waits for one second.
                 sleep(1);
 
                 continue;
             }
 
+            // Sets the lock record with TTL 300 seconds.
             if (! $this->redis->setex($lockKey, 300, (string) Time::now()->getTimestamp())) {
+                // As long as the Redis server is working properly,
+                // the above command will never fail. So the following log will not be recorded.
+                // And if two processes have `$ttl = 0`, both will acquire the lock.
                 $this->logger->error('Session: Error while trying to obtain lock for ' . $this->keyPrefix . $sessionID);
 
                 return false;
@@ -334,6 +343,7 @@ RedisHandle::releaseLock()
     {
         if (isset($this->redis, $this->lockKey) && $this->lock) {
             if (! $this->redis->del($this->lockKey)) {
+                // If two processes have the lock, latter one cannot delete the lock record.
                 $this->logger->error('Session: Error while trying to free lock for ' . $this->lockKey);
 
                 return false;

[motoroller1983]

When this fix will be releaed?

[kenjis]

As soon as possible when this PR is approved and merged.

[najdanovicivan]

@kenjis the only thing I'd add is handling the Redis Exceptions or at least put those in PHPDoc @throws tag

[kenjis]

@najdanovicivan Added @throws.

I also found that we cannot use Redis ACL.

tcp://localhost:6379?auth[user]=username&auth[pass]=password

If it is a bug, I will fix in this PR.

[kenjis]

@najdanovicivan Are you okay to merge this? If so, please approve.

[kenjis]

I also found that we cannot use Redis ACL.

I sent #8578 to 4.5 branch.