Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HttpFoundation] RedisSessionHandler #24781

Merged
merged 1 commit into from Feb 4, 2018

Conversation

@dkarlovi
Copy link
Contributor

dkarlovi commented Nov 1, 2017

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 dkarlovi changed the title Feature/redis session handler WIP: [HttpFoundation] RedisSessionHandler Nov 1, 2017

@dkarlovi dkarlovi force-pushed the dkarlovi:feature/redis-session-handler branch 2 times, most recently from cadbf10 to 0286208 Nov 1, 2017

@dkarlovi

This comment has been minimized.

Copy link
Contributor Author

dkarlovi commented Nov 1, 2017

CI failures unrelated.

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

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 1, 2017

Should target 4.1, thus branch master.

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

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 1, 2017

Member

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

This comment has been minimized.

@dkarlovi

dkarlovi Nov 1, 2017

Author Contributor

Sure, I'll add it.

This comment has been minimized.

@dkarlovi

dkarlovi Nov 1, 2017

Author Contributor

@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?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 1, 2017

Member

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 :)

This comment has been minimized.

@dkarlovi

dkarlovi Nov 1, 2017

Author Contributor

Would we also want to support Array and Cluster too?

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 1, 2017

Member

likely yes :)

This comment has been minimized.

@dkarlovi

dkarlovi Nov 1, 2017

Author Contributor

Oh. :)

@dkarlovi

This comment has been minimized.

Copy link
Contributor Author

dkarlovi commented Nov 1, 2017

@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.

@dkarlovi dkarlovi force-pushed the dkarlovi:feature/redis-session-handler branch from 0286208 to 4e981e5 Nov 1, 2017

@dkarlovi dkarlovi changed the base branch from 3.4 to master Nov 1, 2017

@dkarlovi dkarlovi force-pushed the dkarlovi:feature/redis-session-handler branch from 4e981e5 to 64aa72c Nov 1, 2017

@mvrhov

This comment has been minimized.

Copy link
Contributor

mvrhov commented Nov 1, 2017

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

@dkarlovi

This comment has been minimized.

Copy link
Contributor Author

dkarlovi commented Nov 1, 2017

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

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 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 */

This comment has been minimized.

@Simperfit

Simperfit Nov 1, 2017

Contributor

why inline ?

This comment has been minimized.

@dkarlovi

dkarlovi Nov 1, 2017

Author Contributor

Good question. :) Will fix it.

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 1, 2017

Member

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

This comment has been minimized.

@dkarlovi

dkarlovi Nov 1, 2017

Author Contributor

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 1, 2017

Member

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);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 1, 2017

Member

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';

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 1, 2017

Member

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 1, 2017

Member

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)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 1, 2017

Member

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 1, 2017

Member

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

This comment has been minimized.

@dkarlovi

dkarlovi Nov 1, 2017

Author Contributor

It won't when we start accepting multiple instances.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 1, 2017

Member

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

This comment has been minimized.

@dkarlovi

dkarlovi Nov 1, 2017

Author Contributor

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 1, 2017

Member

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

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Nov 1, 2017

Member

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

This comment has been minimized.

@dkarlovi

dkarlovi Nov 1, 2017

Author Contributor

expire does return bool, yes.

@dkarlovi dkarlovi force-pushed the dkarlovi:feature/redis-session-handler branch from 64aa72c to 35931bd Nov 1, 2017

@mvrhov

This comment has been minimized.

Copy link
Contributor

mvrhov commented Nov 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

This comment has been minimized.

Copy link
Contributor Author

dkarlovi commented Nov 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;

This comment has been minimized.

@javiereguiluz

javiereguiluz Nov 20, 2017

Member

Maybe we can use modern PHP here too?

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

This comment has been minimized.

@dkarlovi

dkarlovi Nov 20, 2017

Author Contributor

@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

This comment has been minimized.

Copy link
Contributor Author

dkarlovi commented Dec 12, 2017

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

This comment has been minimized.

Copy link
Contributor Author

dkarlovi commented Dec 12, 2017

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
Copy link
Member

nicolas-grekas left a comment

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 12, 2017

Member

double param

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 12, 2017

Member

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

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 12, 2017

Member

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

This comment has been minimized.

@dkarlovi

dkarlovi Dec 15, 2017

Author Contributor

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));

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 12, 2017

Member

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

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 12, 2017

Member

no need for $values

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 12, 2017

Member

missing return ''; 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);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 12, 2017

Member

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

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Dec 12, 2017

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

This comment has been minimized.

Copy link
Contributor Author

dkarlovi commented Dec 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.

@dkarlovi dkarlovi force-pushed the dkarlovi:feature/redis-session-handler branch from 7726d37 to 6350afa Dec 15, 2017

@dkarlovi

This comment has been minimized.

Copy link
Contributor Author

dkarlovi commented Dec 15, 2017

@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

This comment has been minimized.

Copy link
Contributor Author

dkarlovi commented Dec 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)) ?? '';

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 19, 2017

Member

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

This comment has been minimized.

@dkarlovi

dkarlovi Dec 19, 2017

Author Contributor

Done.

This comment has been minimized.

@mvrhov

mvrhov Dec 19, 2017

Contributor

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 19, 2017

Member

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

This comment has been minimized.

@mvrhov

mvrhov Dec 20, 2017

Contributor

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 19, 2017

Member

array (null is not allowed)

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

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Dec 19, 2017

Member

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

This comment has been minimized.

@dkarlovi

dkarlovi Dec 20, 2017

Author Contributor

Fixed.

@dkarlovi dkarlovi force-pushed the dkarlovi:feature/redis-session-handler branch from 3c32982 to 45e0858 Dec 20, 2017

@dkarlovi

This comment has been minimized.

Copy link
Contributor Author

dkarlovi commented Dec 20, 2017

Rebased again, can squash before you merge.

@Simperfit

This comment has been minimized.

Copy link
Contributor

Simperfit commented Dec 21, 2017

I think you can squash them now ;).

@dkarlovi

This comment has been minimized.

Copy link
Contributor Author

dkarlovi commented Dec 21, 2017

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

@dkarlovi

This comment has been minimized.

Copy link
Contributor Author

dkarlovi commented Jan 2, 2018

Err, bump?

@mvrhov

This comment has been minimized.

Copy link
Contributor

mvrhov commented Jan 2, 2018

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

@dkarlovi

This comment has been minimized.

Copy link
Contributor Author

dkarlovi commented Jan 29, 2018

@nicolas-grekas can this get merged?

@nicolas-grekas nicolas-grekas force-pushed the dkarlovi:feature/redis-session-handler branch from 45e0858 to 957382b Feb 4, 2018

@nicolas-grekas nicolas-grekas force-pushed the dkarlovi:feature/redis-session-handler branch from 957382b to 8776cce Feb 4, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 4, 2018

Thank you @dkarlovi.

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

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Feb 4, 2018

feature #24781 [HttpFoundation] RedisSessionHandler (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

@dkarlovi dkarlovi deleted the dkarlovi:feature/redis-session-handler branch Feb 5, 2018

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.