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

[Cache] disable lock on CLI #44669

Merged
merged 1 commit into from Dec 16, 2021
Merged

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 16, 2021

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations?
Tickets Fix #41130
License MIT
Doc PR -

I thought using semaphores would fix the linked issue but I'm proposing to revert them in #44667

Instead, I think we should disable the lock on CLI processes.

The purpose of having locks enabled by default is to prevent cache stamped at deploy time. Disabling the lock on CLI still provides this protection on eg FPM.

For ppl that really want a lock between eg crons and FPM, the lock component should be used instead.

@stof
Copy link
Member

@stof stof commented Dec 16, 2021

The issue was talking about problems happening in Apache, so I doubt that disabling it only on CLI will fix it.

@nicolas-grekas
Copy link
Member Author

@nicolas-grekas nicolas-grekas commented Dec 16, 2021

It was about apache calling commands in a subprocess, isn't it?

@stof
Copy link
Member

@stof stof commented Dec 16, 2021

#41130 (comment) should be checked, as it talks about nested caching in parallell requests

@nicolas-grekas
Copy link
Member Author

@nicolas-grekas nicolas-grekas commented Dec 16, 2021

Calling LockRegistry::setFiles([]) will work for these cases.

@nicolas-grekas nicolas-grekas merged commit 311dc83 into symfony:4.4 Dec 16, 2021
10 of 11 checks passed
@nicolas-grekas nicolas-grekas deleted the cache-no-lock-on-cli branch Dec 16, 2021
@fabpot fabpot mentioned this pull request Dec 29, 2021
@fabpot fabpot mentioned this pull request Dec 29, 2021
@fabpot fabpot mentioned this pull request Dec 29, 2021
@fabpot fabpot mentioned this pull request Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants