[HttpFoundation] RedisSessionHandler #24781

dkarlovi · 2017-11-01T10:32:57Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#24433, #18233, #14539, #4538, #3498
License	MIT
Doc PR	symfony/symfony-docs#8572

Ability to use Redis as a session storage backend. Discussed in detail in linked issues / PRs.

dkarlovi · 2017-11-01T11:00:17Z

CI failures unrelated.

nicolas-grekas · 2017-11-01T11:08:20Z

Should target 4.1, thus branch master.

nicolas-grekas · 2017-11-01T11:10:10Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+     *
+     * @throws \InvalidArgumentException When unsupported options are passed
+     */
+    public function __construct(\Redis $redis, array $options = null)


I think this should also support Predis, as the Cache does. There's little effort required to support both drivers.

Sure, I'll add it.

@nicolas-grekas do you mean stuff from RedisTrait? It looks rather complex and I (guess I?) can't use it as it's from another component. Would I need to replicate / inline the whole thing?

Not sure you need to borrow anything, I was just pointing at this to show we already managed to have a single class able to deal with both drivers, so we should be able to make it again here :)

Would we also want to support Array and Cluster too?

likely yes :)

dkarlovi · 2017-11-01T11:17:00Z

@nicolas-grekas

Should target 4.1, thus branch master.

I did that at first, but the PR template said it should target 3.4 so I've changed it. Will switch to master.

mvrhov · 2017-11-01T11:34:33Z

Please check which driver supports locking before supporting it. Without locking the session driver is useless in multi ajax application

dkarlovi · 2017-11-01T11:40:08Z

@mvrhov not sure what you're referring to here exactly, please elaborate.

nicolas-grekas · 2017-11-01T12:31:40Z

@mvrhov neither the Memcached nor the MongoDb do locking. Yet I wouldn't call them useless :) I think it's fine to do no locking here. We could provide locking via a proxy that'd leverage the Lock component BTW. So definitely not in this PR.

Simperfit · 2017-11-01T14:02:11Z

src/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/RedisSessionHandlerTest.php

+     */
+    protected $storage;
+
+    /** @var \PHPUnit_Framework_MockObject_MockObject|\Redis $redis */


why inline ?

Good question. :) Will fix it.

nicolas-grekas · 2017-11-01T16:59:27Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+        // number of deleted items
+        $count = $this->redis->del($this->prefix.$sessionId);
+
+        return 1 === $count;


This is sensitive to race conditions. I think this should always return true instead.

Will change to return true, but FMI, how's this prone to race conditions?

if two concurrent calls to del() are made, one of them will fail because the item will be already deleted

nicolas-grekas · 2017-11-01T17:00:55Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+     */
+    protected function doWrite($sessionId, $data)
+    {
+        return $this->redis->set($this->prefix.$sessionId, $data, $this->ttl);


to increase portability, this should call "setEx" when $ttl > 0

nicolas-grekas · 2017-11-01T17:03:56Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+        }
+
+        $this->ttl = isset($options['expiretime']) ? (int) $options['expiretime'] : 86400;
+        $this->prefix = isset($options['prefix']) ? $options['prefix'] : 'sf2s';


"sf_s" instead of "sf2s"? (just to remove the "2" which doesn't mean anything)

since this supports PHP7 : $options['prefix'] ?? 'sf_s';

nicolas-grekas · 2017-11-01T17:05:39Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+
+        if ($diff = array_diff(array_keys($options), array('prefix', 'expiretime'))) {
+            throw new \InvalidArgumentException(sprintf(
+                'The following options are not supported "%s"', implode(', ', $diff)


exception should be on one line (yes, that's not the case in the Memcached handler, but let's not copy that bad CS)

nicolas-grekas · 2017-11-01T17:06:14Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+class RedisSessionHandler extends AbstractSessionHandler
+{
+    /**
+     * @var \Redis


this docblock should be removed: the constructor already has the info

It won't when we start accepting multiple instances.

it will thanks to the docblock on the constructor, which phpstorm will read, isn't it?

You're right, removing.

nicolas-grekas · 2017-11-01T17:07:24Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+     *  * expiretime: The time to live in seconds.
+     *
+     * @param \Redis $redis   A \Redis instance
+     * @param array  $options An associative array of Redis options


An associative array of ~~Redis~~ options (there's nothing specific to "redis" in these options)

nicolas-grekas · 2017-11-01T17:09:39Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+     */
+    public function updateTimestamp($sessionId, $data)
+    {
+        return $this->redis->expire($this->prefix.$sessionId, $this->ttl);


"expire" return a bool? when supporting both Redis and Predis, this should be tested

expire does return bool, yes.

mvrhov · 2017-11-01T18:41:25Z

@dkarlovi well if the driver doesn't lock the session, and your application is heavily based on ajax and you do a write inside ajax request, then you will probably loose some data.

dkarlovi · 2017-11-01T18:59:55Z

@mvrhov session locking isn't in scope of this PR, I'd like Redis to be usable as a session storage on the same level as other available session handlers (which also means no locking). Session locking should be available across handlers, not just implemented in a single one.

On the other hand, "heavily based on ajax" app (with heavy session write patterns to boot) sounds like a good candidate to do a stateless integration (for example, access tokens).

javiereguiluz · 2017-11-20T07:44:27Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+            throw new \InvalidArgumentException(sprintf('The following options are not supported "%s"', implode(', ', $diff)));
+        }
+
+        $this->ttl = isset($options['expiretime']) ? (int) $options['expiretime'] : 86400;


Maybe we can use modern PHP here too?

$this->ttl = intval($options['expiretime'] ?? 86400);

@javiereguiluz yes, I'll be revamping this once I re-start work on this (which should be shortly).

Is php_cs in master tweaked for Symfony4?

dkarlovi · 2017-12-12T15:02:37Z

I think this should be good to go, I've omitted RedisCluster tests because they seem tricky to setup. Also switched to functional tests instead of unit, seems too complex to mock IMO.

dkarlovi · 2017-12-12T15:04:36Z

Also, if anyone can help me figure out why deps=low fails, that would be great. <3

It seems Travis doesn't make the redis service available with deps=low?

nicolas-grekas

This borrows a lot from the RedisAdapter of the Cache component
Yet it could be simplified since we never deal with multiple values, isn't it?
I think this should be done (but I understand it's not trivial...)

nicolas-grekas · 2017-12-12T16:54:15Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+     *  * prefix: The prefix to use for the keys in order to avoid collision
+     *  * expiretime: The time to live in seconds. Defaults to 1 day.
+     *
+     * @param @param \Redis|\RedisArray|\RedisCluster|\Predis\Client $redisClient


double param

nicolas-grekas · 2017-12-12T16:54:54Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+     *
+     * @throws \InvalidArgumentException When unsupported client or options are passed
+     */
+    public function __construct($redis, ?array $options = null)


the ? should be removed: the arg has a default value, no need to add the same info twice

nicolas-grekas · 2017-12-12T16:55:24Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+        }
+
+        $this->redis = $redis;
+        $options = (array) $options;


if so, just change the default value of the arg and remove this line

This was actually mentioned in #24781 (comment), but since you're the second person to point it out, I've just changed it. 👍

nicolas-grekas · 2017-12-12T16:56:05Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+
+        $diff = array_diff(array_keys($options), array('prefix', 'expiretime'));
+        if ($diff) {
+            $exception = sprintf('The following options are not supported "%s"', implode(', ', $diff));


$message?
but I would suggest to inline and skip the variable

nicolas-grekas · 2017-12-12T16:56:41Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+     */
+    protected function doRead($sessionId): string
+    {
+        $values = $this->doFetch(array($this->prefix.$sessionId));


no need for $values

nicolas-grekas · 2017-12-12T16:57:00Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+
+        foreach ($values as $value) {
+            return $value ?: '';
+        }


missing return ''; after the loop

nicolas-grekas · 2017-12-12T16:57:44Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+    protected function doWrite($sessionId, $data): bool
+    {
+        // will return failed saves (if empty, nothing failed)
+        $values = $this->doSave(array($this->prefix.$sessionId => $data), $this->ttl);


return !$this->doSave(...?

nicolas-grekas · 2017-12-12T17:04:46Z

Yet it could be simplified since we never deal with multiple values, isn't it?

actually, all the pipelining logic should be replaced by simple fetch - pipelining is only usefull when dealing with several values, which is not the case here.

dkarlovi · 2017-12-12T19:06:27Z

@nicolas-grekas I agree, this was to get the tests working for all clients, now it's only the matter of getting the simplest code possible. :)

TYVM for the review.

dkarlovi · 2017-12-15T14:10:12Z

@nicolas-grekas

I've rebased and dropped the whole pipeline thing. Can squash to merge.

deps=low still fails, don't know why. Help?

dkarlovi · 2017-12-19T14:28:20Z

@nicolas-grekas does this look good or do you need me to do anything else?

nicolas-grekas · 2017-12-19T15:11:44Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+     */
+    protected function doRead($sessionId): string
+    {
+        return \unserialize($this->redis->get($this->prefix.$sessionId)) ?? '';


serialization/unserialization should be removed: the passed $data is already serialized by PHP

The serialization is maybe unimportant however if one uses igbinary then some escaping should be done. Unless we don't support it.

@mvrhov can you be more explicit please? I don't understand what you mean.

The "strings" that redis uses are binary safe.. so there is no need to encode the binary data. Sorry for the noise.

nicolas-grekas · 2017-12-19T15:34:45Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+     *  * expiretime: The time to live in seconds. Defaults to 1 day.
+     *
+     * @param \Redis|\RedisArray|\RedisCluster|\Predis\Client $redis
+     * @param null|array                                      $options An associative array of options


array (null is not allowed)

nicolas-grekas · 2017-12-19T15:38:36Z

src/Symfony/Component/HttpFoundation/Session/Storage/Handler/RedisSessionHandler.php

+            throw new \InvalidArgumentException(sprintf('The following options are not supported "%s"', implode(', ', $diff)));
+        }
+
+        $this->ttl = (int) ($options['expiretime'] ?? 86400);


all the other handlers use $maxlifetime = (int) ini_get('session.gc_maxlifetime'); in doWrite/updateTimestamp

dkarlovi · 2017-12-20T16:13:32Z

Rebased again, can squash before you merge.

Simperfit · 2017-12-21T16:25:34Z

I think you can squash them now ;).

dkarlovi · 2017-12-21T18:19:40Z

@Simperfit there might be some more changes requested, will squash when the changeset is confirmed. :)

dkarlovi · 2018-01-02T11:04:57Z

Err, bump?

mvrhov · 2018-01-02T13:44:49Z

@dkarlovi: It's holiday season! A lot of people are home till monday

dkarlovi · 2018-01-29T08:36:24Z

@nicolas-grekas can this get merged?

nicolas-grekas · 2018-02-04T17:23:24Z

Thank you @dkarlovi.

This PR was merged into the 4.1-dev branch. Discussion ---------- [HttpFoundation] RedisSessionHandler | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24433, #18233, #14539, #4538, #3498 | License | MIT | Doc PR | symfony/symfony-docs#8572 Ability to use Redis as a session storage backend. Discussed in detail in linked issues / PRs. Commits ------- 8776cce [HttpFoundation] Add RedisSessionHandler

carsonbot added Status: Needs Review Feature labels Nov 1, 2017

dkarlovi changed the title ~~Feature/redis session handler~~ WIP: [HttpFoundation] RedisSessionHandler Nov 1, 2017

dkarlovi force-pushed the feature/redis-session-handler branch 2 times, most recently from cadbf10 to 0286208 Compare November 1, 2017 10:55

dkarlovi changed the title ~~WIP: [HttpFoundation] RedisSessionHandler~~ [HttpFoundation] RedisSessionHandler Nov 1, 2017

nicolas-grekas added this to the 4.1 milestone Nov 1, 2017

nicolas-grekas reviewed Nov 1, 2017

View reviewed changes

dkarlovi force-pushed the feature/redis-session-handler branch from 0286208 to 4e981e5 Compare November 1, 2017 11:26

dkarlovi changed the base branch from 3.4 to master November 1, 2017 11:27

dkarlovi force-pushed the feature/redis-session-handler branch from 4e981e5 to 64aa72c Compare November 1, 2017 11:32

carsonbot added Status: Reviewed and removed Status: Needs Review labels Nov 1, 2017

Simperfit approved these changes Nov 1, 2017

View reviewed changes

nicolas-grekas reviewed Nov 1, 2017

View reviewed changes

dkarlovi force-pushed the feature/redis-session-handler branch from 64aa72c to 35931bd Compare November 1, 2017 17:55

javiereguiluz reviewed Nov 20, 2017

View reviewed changes

dkarlovi force-pushed the feature/redis-session-handler branch from 35931bd to 143cd82 Compare December 12, 2017 09:51

dkarlovi mentioned this pull request Dec 12, 2017

[Meta] Introduce a static code analyzer (like PHPStan) to the build process #25459

Closed

nicolas-grekas reviewed Dec 12, 2017

View reviewed changes

dkarlovi force-pushed the feature/redis-session-handler branch from 7726d37 to 6350afa Compare December 15, 2017 14:05

nicolas-grekas reviewed Dec 19, 2017

View reviewed changes

dkarlovi force-pushed the feature/redis-session-handler branch from 3c32982 to 45e0858 Compare December 20, 2017 16:12

nicolas-grekas force-pushed the feature/redis-session-handler branch from 45e0858 to 957382b Compare February 4, 2018 17:15

[HttpFoundation] Add RedisSessionHandler

8776cce

nicolas-grekas force-pushed the feature/redis-session-handler branch from 957382b to 8776cce Compare February 4, 2018 17:18

nicolas-grekas approved these changes Feb 4, 2018

View reviewed changes

nicolas-grekas merged commit 8776cce into symfony:master Feb 4, 2018

dkarlovi deleted the feature/redis-session-handler branch February 5, 2018 07:03

fabpot mentioned this pull request May 7, 2018

Release v4.1.0-BETA1 #27181

Merged

[HttpFoundation] RedisSessionHandler #24781

[HttpFoundation] RedisSessionHandler #24781

Conversation

dkarlovi commented Nov 1, 2017 • edited Loading

dkarlovi commented Nov 1, 2017

nicolas-grekas commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkarlovi commented Nov 1, 2017

mvrhov commented Nov 1, 2017

dkarlovi commented Nov 1, 2017

nicolas-grekas commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvrhov commented Nov 1, 2017

dkarlovi commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkarlovi commented Dec 12, 2017

dkarlovi commented Dec 12, 2017 • edited Loading

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Dec 12, 2017 • edited Loading

dkarlovi commented Dec 12, 2017

dkarlovi commented Dec 15, 2017 • edited Loading

dkarlovi commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvrhov Dec 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkarlovi commented Dec 20, 2017

Simperfit commented Dec 21, 2017

dkarlovi commented Dec 21, 2017

dkarlovi commented Jan 2, 2018

mvrhov commented Jan 2, 2018

dkarlovi commented Jan 29, 2018

nicolas-grekas commented Feb 4, 2018

dkarlovi commented Nov 1, 2017 •

edited

Loading

dkarlovi commented Dec 12, 2017 •

edited

Loading

nicolas-grekas commented Dec 12, 2017 •

edited

Loading

dkarlovi commented Dec 15, 2017 •

edited

Loading

mvrhov Dec 19, 2017 •

edited

Loading