Project

General

Profile

Actions

Bug #88866

closed

RedisSessionBackend issue when FE and BE Sessions are in distinct Redis DB

Added by Yann Weyer over 4 years ago. Updated over 4 years ago.

Status:
Closed
Priority:
Should have
Assignee:
-
Category:
Locking / Session Handling
Target version:
-
Start date:
2019-07-30
Due date:
% Done:

100%

Estimated time:
TYPO3 Version:
8
PHP Version:
7.1
Tags:
redis, session,
Complexity:
Is Regression:
Sprint Focus:

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

Actions #1

Updated by Markus Klein over 4 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.

Actions #2

Updated by Gerrit Code Review over 4 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

Actions #3

Updated by Gerrit Code Review over 4 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

Actions #4

Updated by Gerrit Code Review over 4 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

Actions #5

Updated by Yann Weyer over 4 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?

Actions #6

Updated by Markus Klein over 4 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).

Actions #7

Updated by Gerrit Code Review over 4 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

Actions #8

Updated by Gerrit Code Review over 4 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

Actions #9

Updated by Markus Klein over 4 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100
Actions #10

Updated by Gerrit Code Review over 4 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

Actions #11

Updated by Markus Klein over 4 years ago

  • Status changed from Under Review to Resolved
Actions #12

Updated by Benni Mack over 4 years ago

  • Status changed from Resolved to Closed
Actions

Also available in: Atom PDF