[HttpFoundation] Add native handlers for Memcache and Redis

[mevdschee]

Q	A
Branch?	6.3
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	Fix #4976
License	MIT
Doc PR	symfony/symfony-docs#17550

As discussed in ticket #4976 Symfony does not provide Session locking. See also this test suite:

https://github.com/mevdschee/symfony-session-tests

While the documentation does mention race conditions it does not provide a solution. It also fails to mention that the phpredis extension also does not lock by default and that you need to set redis.session.locking_enabled = 1.

To provide a simple and performant solution for session locking I implemented the "native" session handlers for Memcache and Redis.

Some concurrency tests are here: https://github.com/mintyphp/session-handlers

[carsonbot]

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has a contribution guide which I suggest you to read.

In short:

• Always add tests
• Keep backward compatibility (see https://symfony.com/bc).
• Bug fixes must be submitted against the lowest maintained branch where they apply (see https://symfony.com/releases)
• Features and deprecations must be submitted against the 6.3 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

[OskarStark]

Can you please add some tests?

[OskarStark]

Should be final

[mevdschee]

I believe it shouldn't (following the style of the existing code), see also: https://github.com/symfony/symfony/blob/6.3/src/Symfony/Component/HttpFoundation/Session/Storage/Handler/NativeFileSessionHandler.php

[OskarStark]

Should be final

[mevdschee]

Can you please add some tests?

Did you see the extensive test suite? See: https://github.com/mintyphp/session-handlers

I can write some tests like this one: https://github.com/symfony/symfony/blob/6.3/src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/NativeFileSessionHandlerTest.php

Edit: I did write some tests similar to NativeFileSessionHandlerTest

[nicolas-grekas]

Thanks for the PR but I'm wondering: why do we need this when everything is already configurable via php.ini?

[mevdschee]

why do we need this when everything is already configurable via php.ini?

Well, this can be said about the NativeFileSessionHandler as well. What is the reasoning to have that? Is it convenience or safe defaults*? So you can run this on a shared host, where you have no control over the php.ini config? I don't know exactly. I could imagine that this would play a role.

*)Edit: As for the safe defaults argument: The redis session handler that Symfony suggests in the book to be configured in php.ini is not locking in the default configuration, while this is not mentioned.

On the other hand, one could argue that it is mainly the documentation that is steering people away from configuring this in php.ini. If Symfony would say: "we don't do session storage" (as SncRedisBundle did), then people would probably find a well performing and correct path to implement session storage (using PECL implementations). Unfortunately they point to Symfony and then Symfony configures it using PHP classes (not php.ini). Maybe all that is needed is the book to say that session storage must be configured outside Symfony and remove all session storage implementations (including the file native one and the non-locking ones) and add a session locking warning on the redis php.ini default configuration?

[graste]

I like adding stuff that makes session storage with redis or memcached work without further thinking about it, but:
When I e.g. start using symfony in kubernetes with multiple replicas then I need an available session storage. Redis or memcached or database are candidates. So let's say I decide to use Redis. Now I would need to deploy/use a redis cluster to not have it unavailable in the case of redeployments or moving pods around. Now the locking feature the above handler sets (and mentions in the docs PR as working out of the box) no longer works as php/redis doesn't support it. So further improving the docs might be an option?

[mevdschee]

Now I would need to deploy/use a redis cluster to not have it unavailable in the case of redeployments or moving pods around.

@graste I would like to understand your use case better. Redis is a stateful service (even when it is disk-backed). Do you clone the state to the new cluster members during redeployments? Do you reconfigure the clients to let them know about the new cluster members? How does this work in your situation? Can you describe the steps that take place during the (re)deployment?

Edit (additional): The PhpRedis readme writes: "So locking may not work properly in RedisArray or RedisCluster environments.". It is not a limitation of this PR that a primary/secondary (or master/slave as they call it) setup is required for session locking in a Redis cluster. I agree that this could be added to the documentation.

On the other hand: It is similar to the relational database of your symfony application. You can't cluster (stateful) database systems (in general) without configuring them in a primary/secondary setup or losing (some) consistency guarantees. So if you think about it like that, the Redis cluster limitation (with respect to locking or consistency in general) may not be so special after all.

[mevdschee]

It's been a while now, so forgive me to try to summarize: This is not a breaking change, is easy to configure and is (well?) documented. It may benefit people that have no clue about concurrency problems. It works in a small setup (single Redis service or server). It also works in a larger setup (e.g. K8s with multiple PHP nodes and a Redis cluster in primary/secondary mode). That it only works in a primary/secondary Redis cluster setup (as @graste brought up) is similar to the expectations of any other database cluster. Although this caveat could be mentioned in the docs if that is what makes the PR complete. I appreciate any feedback.

[mevdschee]

It has been a while now, so please forgive me to repeat: The Symfony implementations for sessions have lower guarantees than the PHP/native implementations (as the PHP/native implementations have session locking, while the Symfony implementations do not have that), causing very hard to find bugs (losing session persistence in certain edge cases) in real world applications. And note that an unreliable application is worse than a failing one. Even if you don't fix this problem, please update the documentation (except for redis, where it now has the warning) and clearly advice against using the non-locking implementations. On the Redis handler the documentation states:

The main drawback of this solution is that Redis does not perform session locking, so you can face race conditions when accessing sessions. For example, you may see an "Invalid CSRF token" error because two requests were made in parallel and only the first one stored the CSRF token in the session. (source)

But this problem is not Redis specific.

As for performance: The downside of session locking is that concurrent AJAX requests have to wait for each other. This may be a big issue and I understand that seeing AJAX requests wait for each other in the network pane of your developer tools is not nice (when you expect the browser to do 6 concurrent connections per hostname), but you need to realize that this is (in part) a PHP core limitation. It is true that if Symfony or PHP would have had read-only Sessions that there would be no locking needed at all. To realize this the PHP core team has to collaborate on this issue. But even when PHP does lock , then Symfony can release the lock immediately on a read-only session causing almost no performance loss. Symfony could make a "read-only" route that doesn't keep a lock on the session until the request is finished, but releases/writes the session immediately after reading, which is a huge improvement that does require involvement of the PHP core team.

As for importance: It is true that only applications that do AJAX requests that write to the session are impacted. This is true because on page loads browsers serialize the session's HTML requests. It is also true that this bug is a "race condition" that doesn't always occur. IMHO this make the issue more important and not less, as it leads to hard to find bugs.

NB: In order to facilitate good design, I feel that session writes should be disabled (or cause a warning?) on GET requests. For more information, please read: https://tqdev.com/2022-proposal-to-fix-a-2012-bug-in-symfony

NB: You can also watch my presentation from 2015: https://www.youtube.com/watch?v=KDAnIxhsflI

[danydev]

@OskarStark are there some reason to not merge this? Having support for session locking for redis\memached sounds like a "must have" for many applications.