Skip to content

Redis TagAwareCacheInterface stopped working after upgrade to 5.4.2 #44954

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

Closed
madmortigan1 opened this issue Jan 9, 2022 · 27 comments
Closed

Comments

@madmortigan1
Copy link

madmortigan1 commented Jan 9, 2022

Symfony version(s) affected

5.4.2

Description

Hello,

After upgrading to version 5.4.2 the cache service stopped working. I could not trace it to why.
The configuration are working when I downgrade back to 5.4.0.

Unfrtunatly there is no exception that I can catch, and not enough information from nginx, php logs.
Just 502 error from nginx and could not xdebug it, had issues with it.

I went over the documentations and did not see any update that needs to be done to the code or deprecations.

Forgive me for not having enough information accept what I wrote.

Thank you

How to reproduce

Here is the configuration file:
`framework:
cache:
default_redis_provider: 'redis://%env(REDIS_HOST)%:%env(int:REDIS_PORT)%'

    pools:
        redis.cache:
            adapter: 'cache.adapter.redis'
            provider: 'redis://%env(REDIS_HOST)%:%env(int:REDIS_PORT)%'
            default_lifetime: '%framework_cache_lifetime%'
            tags: true

`

I inject it normally view constructor and pool name:
public function __construct( private LoggerInterface $logger, private TagAwareCacheInterface $redisCache ) { }

Usage sample:
$this->redisCache->get(self::CACHE_KEY, function (ItemInterface $item) { $item->expiresAfter(3600); $item->tag(['settings', Core::APP_CACHE_TAG]); ... }

Possible Solution

No response

Additional Context

No response

@kbond
Copy link
Member

kbond commented Jan 10, 2022

Maybe related to #44682 which made adjustments to redis caching with tags. I'll try and take a look later today.

@kbond
Copy link
Member

kbond commented Jan 10, 2022

Hey @madmortigan1, I tried to reproduce best I could based on your description but couldn't duplicate your problem. I'll need a bit more to go on.

Here's my reproducer and the reproduction commit.

@madmortigan1
Copy link
Author

Hi kbond,
Thank you for your help.
I work with docker containers envirounment where redis is configured in the following manner:
`

docker-compose.yaml

redis:
restart: always
image: redis:5.0.5-alpine
container_name: redis_cache
hostname: redis
ports:
- "6379:6379"
networks:
- redis-net
volumes:
- cacheVolumn:/data
`

I did not mention earlier, I will add that I added the following lines to the services.yaml for handling session

`

services.yaml

Redis:
    class: Redis
    calls:
        - method: connect
          arguments:
              - '%env(REDIS_HOST)%'
              - '%env(int:REDIS_PORT)%'
Symfony\Component\HttpFoundation\Session\Storage\Handler\RedisSessionHandler:
    arguments:
        - '@Redis'

`

I saw you worked with predis, I am not familer with it. I did not add any additional redis packages.
If it helps, I will attach my composer.json
composer.zip

What was really buffleing is that I could not debug the issue or catch any exception to try and catch the issue, I checked the redis container to see if the keys were saved anyway but that was not the case.
Xdebug did not show me any variables, it just throw me out when I tried to access ItemInterface (inside the callback).
I actually never encoutered such a strange behaviour with phpstorm where I could letrally not debug or get any kind of exception including in php logs to give some pointer to the issue.

I did try to run bin/console cache:clear which worked without problem, so I am guessing it could connect to the redis container.

Hope this helps to find the issue. I am more than willing to share with you online if it helps to find the issue.

@fancyweb fancyweb added the Cache label Jan 12, 2022
@nicolas-grekas
Copy link
Member

@madmortigan1 could you please put the reproducer in a git repository on github that we could clone and launch easily?

@madmortigan1
Copy link
Author

Yes, I will try to reproduce it and upload it for your convenience.

@madmortigan1
Copy link
Author

Hi,
I have created the envrounment for you guys to work with.
To start working with it please create the docker env:

  • docker-compose up -d --build

composer install inside the docker and go to http://localhost:851/default (or any link you changed to)

Thank you,

@nicolas-grekas
Copy link
Member

@madmortigan1 where is the repo located?

@madmortigan1
Copy link
Author

madmortigan1 commented Jan 25, 2022

@nicolas-grekas Yes, I'll say it was funny on my part.
I thought you get an email automatically when I added you to the repo.

Here is the link:
Repository

@kbond
Copy link
Member

kbond commented Jan 25, 2022

@madmortigan1, can you make it public?

@madmortigan1
Copy link
Author

@kbond I prefare not, but it was done.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jan 25, 2022

Thanks for the reproducer. This looks like a crash of PHP.
This works around the issue apparently, but I don't know why:

--- a/src/Symfony/Component/Cache/Traits/ContractsTrait.php
+++ b/src/Symfony/Component/Cache/Traits/ContractsTrait.php
@@ -42,7 +42,7 @@ trait ContractsTrait
     public function setCallbackWrapper(?callable $callbackWrapper): callable
     {
         if (!isset($this->callbackWrapper)) {
-            $this->callbackWrapper = \Closure::fromCallable([LockRegistry::class, 'compute']);
+            $this->callbackWrapper = [LockRegistry::class, 'compute'];

Please submit a bug report to php-src.

Can you please also try this with symfony/cache 6.0? Is the issue the same?

@nicolas-grekas
Copy link
Member

I patched Symfony in 256ce7f but please follow up this on php-src and link back here when doing so.

@madmortigan1
Copy link
Author

madmortigan1 commented Feb 1, 2022

There are issues with latest php 8.1 container where I am waitting for a critical patch to continue the testing. Apperently they have problems with sockets and the container is not built correctly.
So I am having other issues additionally to this one where the patch is also not working on Symfony 5.4.- now. This is only on the testing envirounment I have created.

I guess I will have to wait a little longer to continue with this issue and stay with what I have now.
Thank you guys for your quick support.

php/php-src#7978

@madmortigan1
Copy link
Author

@nicolas-grekas Hi again,

This issue is still continuing for me.
After updating the php container with their fixes I can run symfony 5.4.6(latest)
Unfortunaly I am unable to upgrade to version 6 cause the same issue actually returns.

I am working on this issue for a very long amount of time without success.
I have no idea how you could debug the cause (probably I have something I can learn from you)
cause it just doesn't give me any logs or break points with xdebug(latest)

I did install a duplicate test site(Symfony 6.0.3) on our normal test server which is not a container and the application works properly.

Seeing that, I have tried to replace the php8.1-fpm-alpine with a different container php8.1-fpm.
Replaces all the installation process but still did not get it to work.
It actually doesn't have anything to do with Redis, it does the same crash if I change it to files.

I cannot find any information regarding this, and not having the error itself even if it PHP crash I do not know what to do next.

Is it possible that we will communicate and I will give you access to my machine and we check this together ?

Any help will be very appreciated here.

@nicolas-grekas
Copy link
Member

This is definitely a php bug, please report it there.

@madmortigan1
Copy link
Author

Yes, but what do I report there ?
I have nothing...

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Mar 15, 2022

You have a reproducer and a workaround, isn't it? That's a very good start. If you can try reducing the reproducer to something as minimal as possible, ppl on php-src would appreciate.

@madmortigan1
Copy link
Author

You are correct, the issue now is when I access the Repository and do a retrieve action inside the cache->get(...)

@madmortigan1
Copy link
Author

You know, I think I finally figured it out or have a lead.
This happens only when I try to access repository inside the cache get class.
entityManagerInterface->getRepository().

And by going line by line since for some unknown reason it does not catch thrown exceptions(wow, have no clue why it does not catches exceptions and saves hours) I found the throwing line:

  • class = PhpFilesAdapter;
  • line: 53
    image

The previous line that gets to the above image is the following:
line: 140 in the same file
image

Now this looks like it searches for a file that does not exists, but it has to do with the Symfony cache.
This happens only if you try to access the repository inside a $cache->get('cache_name'), ....) contract.

I have pushed the reproducing into the same repository.
Added in docker-compose a database

There is a need to create a database named sym6 (otherwise change the name in .env)
You could also use any other database you have, just map an entity to it.

and run the following lines to create the table and add data:

drop table if exists sym6.settings;
create table sym6.settings (
id serial not null,
data text,
primary key (id)
)
with (
oids = false
);

alter table sym6.settings
owner to sym6;

insert into settings ("data")
values ('test1'), ('test2')

@nicolas-grekas
Copy link
Member

I'm afraid we won't be able to do anything here, but that's definitely something worth investigating and reporting to php-src. Please link back here when you'll open a ticket about the issue on php-src.

@madmortigan1
Copy link
Author

madmortigan1 commented Mar 16, 2022

@nicolas-grekas OK, this is deffently the issue.
The "include $file" online 145 throws an uncought error of file not found.
What reason the cache file does not exists is beyond me and you will have better knowledge regarding it.

When debuging it, the files are crated to some point, and than it stops and throws that error.
A very simple fix the resolved it was to check if the file exists first, otherwise do what the catch does:
`
if (self::$valuesCache[$file] ?? false) {
[$expiresAt, $this->values[$id]] = self::$valuesCache[$file];
} else {
if (!file_exists($file)) {
unset($missingIds[$k]);
continue;
}
$expiresAt = include $file;

                    if (\is_array($expiresAt)) {
                        if ($this->appendOnly) {
                            self::$valuesCache[$file] = $expiresAt;
                        }

                        [$expiresAt, $this->values[$id]] = $expiresAt;
                    }
                    elseif ($now < $expiresAt) {
                        $this->values[$id] = new LazyValue($file);
                    }
                }

`
image

I really did all I can here, it's up to you if you want to look into it or not.

I do know the code has not changed for this file specificaly between version 5 to 6.
But other code is effected since in the latest version 5 it works properly.

@madmortigan1
Copy link
Author

@nicolas-grekas
I just saw your answer, we submited on the same time :)

I don't see anything to report to them. It is thrown when trying to include a file that does not exists.
This happens only for entity->getRepository inside cache tag. otherwise the cache works proprly.

@nicolas-grekas
Copy link
Member

The fact that the exception cannot be caught is what you might need to report. The code should be fine without the file_exists check, which is not needed.

@madmortigan1
Copy link
Author

True, and it could be a separate issue.
The question remains how come it works in Symfony 5.* and not 6.
What is the differences there, something is different to cause this error only on version 6?

Would you consider patching that work around anyways so I can upgrade and continue to work ?

@nicolas-grekas
Copy link
Member

I don't think committing a workaround in Symfony would be a good idea, unless we precisely understand the issue and decide we have no choice. But at this stage, reporting+fixing this on php-src is what would be the most useful for the php community.

@madmortigan1
Copy link
Author

madmortigan1 commented Mar 17, 2022

@nicolas-grekas I submitted bug to php rpc Here
I would like to add here that I tried to reproduce it with a loop for over 1500 retries to catch imagined files and it always catched the error.

In symfony 6 it throws segment 11 fault that is thrown, the ltrace, strace didn't give me any clues or understanding whats behind it.

Also strange no one else complained about it.

@mrsuh
Copy link
Contributor

mrsuh commented Apr 12, 2022

Hi guys!
I found a way to reproduce the error:

composer install symfony/cache

test.php

<?php

use Symfony\Component\Cache\Adapter\FilesystemAdapter;

require __DIR__ . '/vendor/autoload.php';

$cache = new FilesystemAdapter();
$cache->get('wrong@key', function () {});
docker run -it --rm -v $PWD:/app -w /app php:8.1.2-cli bash
pecl install xdebug-3.1.3
docker-php-ext-enable xdebug
php test.php

The error only reproduces with:

  • PHP 8.1.0 - 8.1.3
  • Xdebug <= 3.1.3

The error comes from creating \Closure here with a class with static variables here

Bug is already fixed in PHP and Xdebug

I think that we can fix it in the library code because it will fix the error and I think that using static variables in class functions is not a good practice.
Class static properties and class function static variables has similar behaviour, but in second case it's not clear to read.
Static variables are global variables and I want to see all these variables at the beginning of class so that I know all the class contracts and what variable names I can use.

If you agree with me, I made a PR

nicolas-grekas added a commit that referenced this issue Apr 12, 2022
…of static variables (mrsuh)

This PR was submitted for the 5.4 branch but it was merged into the 4.4 branch instead.

Discussion
----------

[Cache] make LockRegistry use static properties instead of static variables

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #44954
| License       | MIT

Commits
-------

3b6a56d [Cache] make LockRegistry use static properties instead of static variables
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants