Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] RedisSessionHandler#24781
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[HttpFoundation] RedisSessionHandler#24781
Uh oh!
There was an error while loading.Please reload this page.
Conversation
cadbf10 to0286208Comparedkarlovi commentedNov 1, 2017
CI failures unrelated. |
nicolas-grekas commentedNov 1, 2017
Should target 4.1, thus branch master. |
| * | ||
| * @throws \InvalidArgumentException When unsupported options are passed | ||
| */ | ||
| public function __construct(\Redis $redis, array $options = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think this should also support Predis, as the Cache does. There's little effort required to support both drivers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure, I'll add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@nicolas-grekas do you mean stuff fromRedisTrait? 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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Would we also want to support Array and Cluster too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
likely yes :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh. :)
dkarlovi commentedNov 1, 2017
I did that at first, but the PR template said it should target 3.4 so I've changed it. Will switch to master. |
0286208 to4e981e5Compare4e981e5 to64aa72cComparemvrhov commentedNov 1, 2017
Please check which driver supports locking before supporting it. Without locking the session driver is useless in multi ajax application |
dkarlovi commentedNov 1, 2017
@mvrhov not sure what you're referring to here exactly, please elaborate. |
nicolas-grekas commentedNov 1, 2017
@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. |
| */ | ||
| protected $storage; | ||
| /** @var \PHPUnit_Framework_MockObject_MockObject|\Redis $redis */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why inline ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Good question. :) Will fix it.
| // number of deleted items | ||
| $count = $this->redis->del($this->prefix.$sessionId); | ||
| return 1 === $count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is sensitive to race conditions. I think this should always return true instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Will change toreturn true, but FMI, how's this prone to race conditions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if two concurrent calls to del() are made, one of them will fail because the item will be already deleted
| */ | ||
| protected function doWrite($sessionId, $data) | ||
| { | ||
| return $this->redis->set($this->prefix.$sessionId, $data, $this->ttl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
to increase portability, this should call "setEx" when $ttl > 0
| } | ||
| $this->ttl = isset($options['expiretime']) ? (int) $options['expiretime'] : 86400; | ||
| $this->prefix = isset($options['prefix']) ? $options['prefix'] : 'sf2s'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"sf_s" instead of "sf2s"? (just to remove the "2" which doesn't mean anything)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
since this supports PHP7 :$options['prefix'] ?? 'sf_s';
| if ($diff = array_diff(array_keys($options), array('prefix', 'expiretime'))) { | ||
| throw new \InvalidArgumentException(sprintf( | ||
| 'The following options are not supported "%s"', implode(', ', $diff) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
exception should be on one line (yes, that's not the case in the Memcached handler, but let's not copy that bad CS)
| class RedisSessionHandler extends AbstractSessionHandler | ||
| { | ||
| /** | ||
| * @var \Redis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this docblock should be removed: the constructor already has the info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It won't when we start accepting multiple instances.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it will thanks to the docblock on the constructor, which phpstorm will read, isn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You're right, removing.
| * * expiretime: The time to live in seconds. | ||
| * | ||
| * @param \Redis $redis A \Redis instance | ||
| * @param array $options An associative array of Redis options |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
An associative array ofRedis options (there's nothing specific to "redis" in these options)
| */ | ||
| public function updateTimestamp($sessionId, $data) | ||
| { | ||
| return $this->redis->expire($this->prefix.$sessionId, $this->ttl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
"expire" return a bool? when supporting both Redis and Predis, this should be tested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
expire does return bool, yes.
64aa72c to35931bdComparemvrhov commentedNov 1, 2017
@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 commentedNov 1, 2017
@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). |
| throw new \InvalidArgumentException(sprintf('The following options are not supported "%s"', implode(', ', $diff))); | ||
| } | ||
| $this->ttl = isset($options['expiretime']) ? (int) $options['expiretime'] : 86400; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Maybe we can use modern PHP here too?
$this->ttl =intval($options['expiretime'] ??86400);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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?
35931bd to143cd82Comparedkarlovi commentedDec 12, 2017
I think this should be good to go, I've omitted |
dkarlovi commentedDec 12, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Also, if anyone can help me figure out why It seems Travis doesn't make the |
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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...)
| * * 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
double param
| * | ||
| * @throws \InvalidArgumentException When unsupported client or options are passed | ||
| */ | ||
| public function __construct($redis, ?array $options = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
the? should be removed: the arg has a default value, no need to add the same info twice
| } | ||
| $this->redis = $redis; | ||
| $options = (array) $options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if so, just change the default value of the arg and remove this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This was actually mentioned in#24781 (comment), but since you're the second person to point it out, I've just changed it. 👍
| $diff = array_diff(array_keys($options), array('prefix', 'expiretime')); | ||
| if ($diff) { | ||
| $exception = sprintf('The following options are not supported "%s"', implode(', ', $diff)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$message?
but I would suggest to inline and skip the variable
| */ | ||
| protected function doRead($sessionId): string | ||
| { | ||
| $values = $this->doFetch(array($this->prefix.$sessionId)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no need for $values
| foreach ($values as $value) { | ||
| return $value ?: ''; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
missingreturn ''; after the loop
| protected function doWrite($sessionId, $data): bool | ||
| { | ||
| // will return failed saves (if empty, nothing failed) | ||
| $values = $this->doSave(array($this->prefix.$sessionId => $data), $this->ttl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
return !$this->doSave(...?
nicolas-grekas commentedDec 12, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 commentedDec 12, 2017
@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. |
7726d37 to6350afaComparedkarlovi commentedDec 15, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I've rebased and dropped the whole pipeline thing. Can squash to merge.
|
dkarlovi commentedDec 19, 2017
@nicolas-grekas does this look good or do you need me to do anything else? |
| */ | ||
| protected function doRead($sessionId): string | ||
| { | ||
| return \unserialize($this->redis->get($this->prefix.$sessionId)) ?? ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
serialization/unserialization should be removed: the passed $data is already serialized by PHP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The serialization is maybe unimportant however if one uses igbinary then some escaping should be done. Unless we don't support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@mvrhov can you be more explicit please? I don't understand what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The "strings" that redis uses are binary safe.. so there is no need to encode the binary data. Sorry for the noise.
| * * 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
array (null is not allowed)
| throw new \InvalidArgumentException(sprintf('The following options are not supported "%s"', implode(', ', $diff))); | ||
| } | ||
| $this->ttl = (int) ($options['expiretime'] ?? 86400); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
all the other handlers use$maxlifetime = (int) ini_get('session.gc_maxlifetime'); in doWrite/updateTimestamp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Fixed.
3c32982 to45e0858Comparedkarlovi commentedDec 20, 2017
Rebased again, can squash before you merge. |
Simperfit commentedDec 21, 2017
I think you can squash them now ;). |
dkarlovi commentedDec 21, 2017
@Simperfit there might be some more changes requested, will squash when the changeset is confirmed. :) |
dkarlovi commentedJan 2, 2018
Err, bump? |
mvrhov commentedJan 2, 2018
@dkarlovi: It's holiday season! A lot of people are home till monday |
dkarlovi commentedJan 29, 2018
@nicolas-grekas can this get merged? |
45e0858 to957382bCompare957382b to8776cceComparenicolas-grekas commentedFeb 4, 2018
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#8572Ability to use Redis as a session storage backend. Discussed in detail in linked issues / PRs.Commits-------8776cce [HttpFoundation] Add RedisSessionHandler
Uh oh!
There was an error while loading.Please reload this page.
Ability to use Redis as a session storage backend. Discussed in detail in linked issues / PRs.