Bug #88866

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

Added by Yann Weyer 6 months ago. Updated about 1 month ago.

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

100%

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

Associated revisions

Revision 962b3cd8 (diff)
Added by Markus Klein 6 months ago

[BUGFIX] Make redis pconnect calls unique

By using the persistent_id parameter of the redis->pconnect
method, we ensure that the connection is not shared between
multiple Redis*Backends connecting to the same Redis server.

Omitting the persistent_id causes the same connection to be reused
whenever another Redis*Backend is created, whereby the last
connection selects the database to use for the connection, effectively
causing all Redis*Backends to write to the same database.

The RedisSessionBackend uses the pconnect method by default and
therefore requires this fix in order to distinguish FE and BE
backends correctly, if both are stored within a Redis database.

The pconnect is optional for the cache RedisBackend, but we still
use the database number now for the persistent_id parameter.

Resolves: #88866
Releases: master, 9.5, 8.7
Change-Id: I987c36e89f2ab53fd5177cdc7051811b116bcad0
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/61393
Tested-by: TYPO3com <>
Tested-by: Andreas Fernandez <>
Tested-by: Frank Naegler <>
Reviewed-by: Oliver Klee <>
Reviewed-by: Andreas Fernandez <>
Reviewed-by: Frank Naegler <>

Revision ff26e6b2 (diff)
Added by Markus Klein 6 months ago

[BUGFIX] Make redis pconnect calls unique

By using the persistent_id parameter of the redis->pconnect
method, we ensure that the connection is not shared between
multiple Redis*Backends connecting to the same Redis server.

Omitting the persistent_id causes the same connection to be reused
whenever another Redis*Backend is created, whereby the last
connection selects the database to use for the connection, effectively
causing all Redis*Backends to write to the same database.

The RedisSessionBackend uses the pconnect method by default and
therefore requires this fix in order to distinguish FE and BE
backends correctly, if both are stored within a Redis database.

The pconnect is optional for the cache RedisBackend, but we still
use the database number now for the persistent_id parameter.

Resolves: #88866
Releases: master, 9.5, 8.7
Change-Id: I987c36e89f2ab53fd5177cdc7051811b116bcad0
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/61454
Tested-by: TYPO3com <>
Tested-by: Frank Naegler <>
Reviewed-by: Frank Naegler <>

Revision 2bc6e5b8 (diff)
Added by Markus Klein 6 months ago

[BUGFIX] Make redis pconnect calls unique

By using the persistent_id parameter of the redis->pconnect
method, we ensure that the connection is not shared between
multiple Redis*Backends connecting to the same Redis server.

Omitting the persistent_id causes the same connection to be reused
whenever another Redis*Backend is created, whereby the last
connection selects the database to use for the connection, effectively
causing all Redis*Backends to write to the same database.

The RedisSessionBackend uses the pconnect method by default and
therefore requires this fix in order to distinguish FE and BE
backends correctly, if both are stored within a Redis database.

The pconnect is optional for the cache RedisBackend, but we still
use the database number now for the persistent_id parameter.

Resolves: #88866
Releases: master, 9.5, 8.7
Change-Id: I987c36e89f2ab53fd5177cdc7051811b116bcad0
Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/61455
Tested-by: TYPO3com <>
Tested-by: Frank Naegler <>
Tested-by: Susanne Moog <>
Reviewed-by: Frank Naegler <>
Reviewed-by: Susanne Moog <>

History

#1 Updated by Markus Klein 6 months 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.

#2 Updated by Gerrit Code Review 6 months 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

#3 Updated by Gerrit Code Review 6 months 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

#4 Updated by Gerrit Code Review 6 months 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

#5 Updated by Yann Weyer 6 months 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?

#6 Updated by Markus Klein 6 months 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).

#7 Updated by Gerrit Code Review 6 months 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

#8 Updated by Gerrit Code Review 6 months 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

#9 Updated by Markus Klein 6 months ago

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

#10 Updated by Gerrit Code Review 6 months 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

#11 Updated by Markus Klein 6 months ago

  • Status changed from Under Review to Resolved

#12 Updated by Benni Mack about 1 month ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF