Bug #88866
closedRedisSessionBackend issue when FE and BE Sessions are in distinct Redis DB
100%
Description
Symptoms¶
When using two distinct Sessions databases on the same Redis server, we observed FE keys in the BE database. It happens as soon as users have both a BE and a FE session, when extensions try to save data in the FE session.
Another user reported a similar problem in https://github.com/TYPO3-Solr/ext-solr/issues/1928
Cause¶
Debugging it a bit further, I isolated the following line in RedisSessionBackend.php :
try {
$this->connected = $this->redis->pconnect(
$this->configuration['hostname'] ?? '127.0.0.1',
$this->configuration['port'] ?? 6379
);
} catch (\RedisException $e) {
$this->logger->alert('Could not connect to redis server.', ['exception' => $e]);
}
Although I can't exactly explain why, it seems pconnect
automatically shares the Redis connection between the two distinct Sessions backends, resulting in data being read or written in the wrong database.
Possible fixes¶
Use connect
instead of pconnect
¶
By replacing $this->redis->pconnect
by $this->redis->connect
, sessions are saved in correct database as intended. The drawback of this option is it might harm the performance under high load.
Define a persistent_id
for each database¶
To ensure Redis connection stay independant, pconnect offers a persistent_id to tell them apart. It might make sense to include the Redis target database to said id:
$this->connected = $this->redis->pconnect(
$this->configuration['hostname'] ?? '127.0.0.1',
$this->configuration['port'] ?? 6379,
0, /* timeout parameter (0 by default) */
get_class($this) . $this->configuration['database'] /* persistent_id: class + target database */
);
Using get_class($this) . $this->configuration['database']
as a persistent_id
is only a test from me, perhaps it would make sense to keep it more general? (e.g: a hash of hostname + port + database?)
It might also worth checking if the Redis caching backend might benefit from a similar
persistent_id
introduction when using persistent connections.
Mention this as a requirement in the documentation¶
If, for any reason, it is intended to have only one database for both BE and FE sessions, I think it should be mentioned in the documentation of the SessionStorageFramework
Updated by Markus Klein over 5 years ago
Interesting finding. Thanks for the report.
Reading your lines things are actually pretty clear to me:
Each instance of the RedisSessionBackend is effectively reusing the same existing connection, due to the fact (like you pointed out) that phpredis has 3 strategies to identify a connection:
- host + port + timeout, or
- host + persistent_id, or
- unix socket + timeout
Since you use the same server the options 1 and 3 effectively always use the same connection for both BE and FE session backend. The one which selects the database last of course determines the database used on this connection.
As a fix it would most probably suffice to define the persistent_id
as $this->identifier
to make things distinguishable.
Updated by Gerrit Code Review over 5 years ago
- Status changed from New to Under Review
Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61393
Updated by Gerrit Code Review over 5 years ago
Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61393
Updated by Gerrit Code Review over 5 years ago
Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61393
Updated by Yann Weyer over 5 years ago
Hi Markus,
Thank you for your quick answer :)
If I understand correctly, using $this->identifier
as persistent_id
would force PHP to open two connections (one for FE
, and another one for BE
) even with only one Redis DB (which I think is the default and documented setting). I am not sure how the performance might be affected in such cases.
Wouldn't $this->configuration['database']
be an adequate alternative? If it is the same, then only one connection gets opened, as it used to be; if not, each backend gets its own connection to the appropriate database.
Am I perhaps missing something?
Updated by Markus Klein over 5 years ago
Hi!
Yes of course we could use the database number as well, but I don't think it is a problem to have more than one connection open.
Please feel free to add your comments directly with the patch on the review platform, so other reviewers see them too (not everybody is checking the ticket here all the time).
Updated by Gerrit Code Review over 5 years ago
Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61393
Updated by Gerrit Code Review over 5 years ago
Patch set 1 for branch 9.5 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61454
Updated by Markus Klein over 5 years ago
- Status changed from Under Review to Resolved
- % Done changed from 0 to 100
Applied in changeset 962b3cd8c02400e14d9d79e28c2aa9f7af90c343.
Updated by Gerrit Code Review over 5 years ago
- Status changed from Resolved to Under Review
Patch set 1 for branch TYPO3_8-7 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/c/Packages/TYPO3.CMS/+/61455
Updated by Markus Klein over 5 years ago
- Status changed from Under Review to Resolved
Applied in changeset ff26e6b2ae67fefde23e288d9dad0338161893ef.
Updated by Benni Mack almost 5 years ago
- Status changed from Resolved to Closed