2.1.5 (lazy loading) somehow breaks set method on session client

[sawmurai]

Hey guys,

the lazy-loading introduced with the latest patch somehow seems to break the set method.

When calling it, the following error pops up:

Redis::set() expects at most 3 parameters, 4 given

although it gets called with 3 parameters (in a controller):

    private function setRefreshTokenCachedResponse(Request $request, Response $response)
    {
        $redis = $this->container->get('snc_redis.session');
        $redis->set(
            $request->get('refresh_token'),
            $response->getContent(),
            self::REFRESH_TOKEN_RESPONSE_CACHE_LIFETIME
        );
    }

[dkarlovi]

Talking about it with @Ocramius here: https://twitter.com/dkarlovi/status/1020271285320941568

[Ocramius]

Please also show the generated code for the proxy.

[sawmurai]

Redis_ca5fc0f.php.txt

You mean this?

[dkarlovi]

https://gist.github.com/dkarlovi/cd0174593bd6b0873afc803bd5a43978

[dkarlovi]

So, as noted:

Timeout or Options Array (optional). If you pass an integer, phpredis will redirect to SETEX, and will try to use Redis >= 2.6.12 extended options if you pass an array with valid values

It should be one of, they're both included for some reason.

[Ocramius]

Yep, exactly that.

And holy crap this class has an atrocious API: just how many responsibilities are crammed together here?!

Besides that, as you can see, set() is generated with 4 parameters, of which 2 optional.

To keep in mind when going further here:

• ext-reflection is seeing 4 parameters here
• default parameter values of methods defined in core or in an extension cannot be reflected, hence NULL as default value.

[Ocramius]

Btw, this is neither a bug in ProxyManager nor in this bundle: you will need to either fix this in the extension, or provide a userland shim that can be reflected.

[dkarlovi]

Alright, thanks. I guess we need to open an issue with Phpredis.

[sawmurai]

Thanks for the quick feedback guys. Highly appreciated!

[curry684]

Yep, phpredis is a mess, it's essentially just all Redis commands thrown together in a big bucket class. Considering that's how Redis itself works it's not even unreasonable but it makes wrapping hell.

Marking this issue as a not-our-problem-but-we-need-to-keep-it-in-mind issue.

[dkarlovi]

Opened the issue: phpredis/phpredis#1374

[curry684]

@dkarlovi how do you propose we handle compatibility on this until and after phpredis has released a fix? Should/could we just prohibit the lazy loading in "broken" phpredis versions?

[Ocramius]

As mentioned above: provide a userland class that has the complete signature

[dkarlovi]

@Ocramius any pointers how we would provide such a class from a bundle?

[Ocramius]

By writing it? O_o

[curry684]

Once reflection data is correct we can generate it during container compilation from that, which is actually a good idea anyway because it's also the only way I see to re-enable the profiler wrappers.

[Ocramius]

I suggest doing it at least for a new minor

[curry684]

@lashae if using lazy services not until you are running the new phpredis version which may be released later tonight.

[dkarlovi]

Pushed a toy to test reflection against released and develop phpredis: https://github.com/dkarlovi/phpredis-reflection

[dkarlovi]

@Ocramius this is my first time using both ProxyManager and BetterReflection so it's probably broken in more than one way, but it seems there's issues with either BetterReflection or ProxyManager with certain methods.

Example:

}Method [ <internal:redis> public method georadius ] {

  - Parameters [6] {
    Parameter #0 [ <required> $key ]
    Parameter #1 [ <required> $lng ]
    Parameter #2 [ <required> $lan ]
    Parameter #3 [ <required> $radius ]
    Parameter #4 [ <required> $unit ]
    Parameter #5 [ <optional> array or NULL $opts = NULL ]
  }
}
Fatal error: Uncaught TypeError: Argument 6 passed to Redis::georadius() must be of the type array, null given in /test/vendor/ocramius/proxy-manager/src/ProxyManager/GeneratorStrategy/EvaluatingGeneratorStrategy.php(54) : eval()'d code:302
Stack trace:
#0 /test/vendor/ocramius/proxy-manager/src/ProxyManager/GeneratorStrategy/EvaluatingGeneratorStrategy.php(54) : eval()'d code(302): Redis->georadius(NULL, NULL, NULL, NULL, NULL, NULL)
#1 /test/reflect.php(66): ProxyManagerGeneratedProxy\__PM__\Redis\Generatedd0ce2cf3bafa1792c1dbd369ceca90fc->georadius(NULL, NULL, NULL, NULL, NULL)
#2 {main}
  thrown in /test/vendor/ocramius/proxy-manager/src/ProxyManager/GeneratorStrategy/EvaluatingGeneratorStrategy.php(54) : eval()'d code on line 302

[Ocramius]

Most likely typical PHP extension fuckery.

In PHP extensions, it is possible to have an optional parameter that also considers the missing parameter NULL, even if the parameter is not nullable. This means that a PHP userland signature for this cannot exist.

Example:

function foo(array $bar = []){}

In PHP core, you can decide that foo() is equivalent to foo(null), even though such a call would be illegal in userland (it would be foo([]) for us mere mortals).

The other possibility is that the method is completely mis-documented, and you will need to experiment with combinations of allowed parameters.

[dkarlovi]

Issue fixed with Phpredis 4.1.1.

The bundle should still provide a stub for users who don't have this version installed, though.

[dkarlovi]

I can confirm this fully works with Phpredis 4.1.1 within my app. If you're using Alpine, you can already find it packaged for Edge.

[B-Galati]

Thank you all! Amazing work :-)

[ondrejfuhrer]

Nice, I was wondering why the application cashes when updated a minor version of this bundle 😄
Just an idea: is there a particular reason why the definition is forced as lazy? When I updated that it worked fine also with older phpredis version.

So maybe having the option to disable the lazy loading of the client via the configuration would fix that regardless the phpredis version?

[B-Galati]

@ondrejfuhrer Which issue are you talking about exactly? This one or the cache warmup issue with redis server available?

The lazyness was introduced because phpredis object creation was creating a connection to Redis server even if you don't really need it. For example when you warm up the cache.

This was happening only with some symfony configuration.

We could always add an opt-out option for lazyness indeed, but not sure it worth it.

I quickly checked Symfony codebase for the redis cache adapter and it looks like the lazyness is implemented by using \Symfony\Component\Cache\Traits\RedisProxy. Not sure about everything here.

Cheers.

[curry684]

Just an idea: is there a particular reason why the definition is forced as lazy?

It's not forced as lazy, we're just supporting a feature in Symfony now we should have been doing before. We were just not aware of the bug in phpredis < 4.1.1 we uncovered with it, hence why the "breaking change" occurred in a minor.

[ondrejfuhrer]

@B-Galati @curry684
I'm talking about this line: https://github.com/snc/SncRedisBundle/blob/master/DependencyInjection/SncRedisExtension.php#L279

If that would be written in a way:

$lazyClient = $container->getParameter('snc_redis.phpredis_client.lazy') ?? true;
...
$phpredisDef->setLazy($lazyClient);

Then even for people who cannot for whatever reason upgrade Phpredis fast enough, as a tradeoff you can disable the lazy loading and work with the latest bundle version.

I have confirmed myself that changing that line is indeed "fixing" the issue wit not passed arguments to the get / set / ... functions.

[B-Galati]

@ondrejfuhrer Thank you

I think we could do two things:

• Add an option to opt-out from lazy loading to let one do as he likes
• Do what is mentioned in this comment 2.1.5 (lazy loading) somehow breaks set method on session client #442 (comment)

[ondrejfuhrer]

@B-Galati Also one thing to mention -> there is also inconsistency in loading the clients based on what type you use (I would personally prefer consistency there). What I mean is that right now the Phpredis client is loaded as lazy and Predis client is not.

So it would also make sense from consistency perspective to have both affected by that option.

[B-Galati]

@ondrejfuhrer No I don't think so because Predis client is lazy by default: it does not try to connect to redis unless something is asked.
With PHPRedis you have two method to connect: pconnect and connect. To support both we call the choosen one on service creation, thus it tries to connect to redis. To avoid useless connection we added laziness to the service itself. It's the easiest solution.
So IMHO, it would be useless to add laziness to Predis client. Consistency is not a strong argument in that case.

[damijank]

Here's some additional intel on the matter of the lazy loading problem:

Phpredis was recently upped to 4.2.0. A change in 4.2.0RC2 introduces correct reflection for the Redis::multi() method - one optional argument. The problem arises from the fact that reflection can not retrieve default values of an optional argument if the method is internal, and lazy loading creates a virtual proxy class with information retrieved via reflection. That means the multi() method in proxy is defined as:

public function multi($mode = null)

whereas the real default value for $mode is Redis::MULTI.

This means when the service is lazy loaded, and we use the method without arguments, the actual method is called like multi(null) (instead of without arguments, or with the correct default value for the argument).

I understand we can create our own stub and (re)introduce default values in a user defined class which would result in a perfect reflection info and in turn a 1:1 proxy class. But, there are many versions of Phpredis to support. How to handle many cases? Whose responsibility would it be to write a correct stub for (every) Phpredis release? I assume there would be a way to have this bundle use the correct stub, based on ext-redis version, but this seems like a heavy undertaking with synchronicity on both sides.

Further, with older versions of Phpredis (ie. 4.0.2) this particular method did not have any ARGINFO present so based on the reflection information, the optional argument was never forwarded from the proxy class to the real class. One could literally call the method like multi(new \DateTime()) and the actual method would still recieve the call without arguments, thus (unintentionally) using its own default.

This is a general issue with proxies for internal classes, maybe Symfony should not create proxies for them at all? Even if everything is done right on the side of Phpredis (which is not, but let's assume) - still there is the fact that reflection does not have the ability to get default values for optional arguments of internal methods. This change would render your lazy loading technique futile though.

All this said, I think the most immediate solution would be to add a lazy parameter to client configurations... this was already proposed but somehow dismissed. I hope I've given enough more insight why this should actually be implemented.

Partially, this is discussed in phpredis/phpredis#1476 too.

[dkarlovi]

As noted in phpredis/phpredis#1476 (comment), this is fully on phpredis, not on this bundle or on Symfony.

[Ocramius]

Setting the default to public function multi($mode = Redis::MULTI) is only currently possible by subclassing in userland, then pointing at that class in the service definitions. I've described it above in #442 (comment)

[B-Galati]

In Symfony, they use a special kind of laziness. I guess it allows to bypass some weird edge cases like we have right now:
https://github.com/symfony/symfony/blob/da4019a4be54bc01f8e25532c9b3933b2633181e/src/Symfony/Component/Cache/Traits/RedisTrait.php#L190-L194

I would love to use this RedisTrait but it is marked as @internal sadly.
Should we copy/paste it in Snc/RedisBundle for v3 and use it to create Redis clients?

[dkarlovi]

@B-Galati maybe asking @nicolas-grekas directly is best?

[nicolas-grekas]

Copy/pasting is fine really.

[B-Galati]

Thanks @nicolas-grekas and @dkarlovi!
I will try to do something in coming weeks / months.

[ostrolucky]

I think this has been fixed in #507