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

Handle custom lock file during the cache clear and force module actions one by one #31820

Merged
merged 3 commits into from Mar 23, 2023

Conversation

jolelievre
Copy link
Contributor

@jolelievre jolelievre commented Mar 17, 2023

Questions Answers
Branch? 8.0.x
Description? Handle custom lock file during the cache clear to make sure other processes have to wait until the new container is ready This is an alternative to the solution proposed in this PR #31419
Type? bug fix
Category? BO
BC breaks? no
Deprecations? yes
How to test? The lock happens when Symfony cache is clear, mainly during any module operation (enable, disable, install, uninstall, ...) During the cache clearing, any other process (from a different tab) should be blocked until the cache is available again. We need to make sure that the shop doesn't end up in a blocked state for some reason, and we also should probably test that this doesn't impact the auto-upgrade process It could also be interesting to check that the use case related to this PR #30962 is still valid as it was also a complex use case related to cache clearing

The module management has also been modified, now the bulk actions are forced one by one and when you try to perform two actions at the same time (by clicking on a button for a module while an action is in progress on another) the action is blocked and a warning message warns you that you need to wait
Fixed ticket? Fixes #31562 and Fixes #28304
Related PRs ~
Sponsor company ~

@jolelievre jolelievre requested a review from a team as a code owner March 17, 2023 13:43
@prestonBot prestonBot added 8.0.x Branch Bug fix Type: Bug fix labels Mar 17, 2023
@jolelievre
Copy link
Contributor Author

jolelievre commented Mar 17, 2023

(Edit 20/03 3h30pm) Running campaign https://github.com/jolelievre/ga.tests.ui.pr/actions/runs/4469351523


// Unlock is registered in another seaprate function to make sure it will be called no matter what
register_shutdown_function(function () use ($kernel) {
$kernel->unlocksCacheClear();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you call exit() within one registered shutdown function, processing will stop completely and no other registered shutdown functions will be called.
https://www.php.net/manual/en/function.register-shutdown-function.php

What if some code tied to actionClearSf2Cache hook executes an exit() call ? Maybe we could have a max_locking_time as a fallback ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean having a timeout after which the file is unlocked? There is no such feature with flock as far as I know.

But anyway even if the file is not unlocked manually it will be automatically unlocked after the process that locked it ends, so if indeed someone calls exit in actionClearSf2Cache even though they SHOULD NEVER DO IT then the file will be unlocked.

Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pragmatic enough for me for a patch 👍

src/Adapter/Cache/Clearer/SymfonyCacheClearer.php Outdated Show resolved Hide resolved
…cesses have to wait until the new container is ready
zuk3975
zuk3975 previously approved these changes Mar 20, 2023
@jolelievre jolelievre requested a review from matks March 21, 2023 08:56
matks
matks previously approved these changes Mar 21, 2023
@jolelievre jolelievre added the Waiting for QA Status: action required, waiting for test feedback label Mar 21, 2023
…rrent one is finished and bulk actions are launched in sequence not in parallel
@jolelievre jolelievre dismissed stale reviews from matks and zuk3975 via 7f65acd March 21, 2023 13:32
@prestonBot
Copy link
Collaborator

This pull request seems to contain new translation strings. I have summarized them below to ease up review:

(Note: this is an automated message, but answering it will reach a real human)

@prestonBot prestonBot added the Waiting for wording Status: action required, waiting for wording label Mar 21, 2023
@jolelievre jolelievre removed the Waiting for QA Status: action required, waiting for test feedback label Mar 21, 2023
@jolelievre jolelievre changed the title Handle custom lock file during the cache clear to make sure other pro… Handle custom lock file during the cache clear and force module actions one by one Mar 21, 2023
Copy link
Contributor

@mflasquin mflasquin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but as I understand we will clear the cache for all actions (if clear cache is finish on previous action) on modules ? Not only the last action ?

@jolelievre jolelievre added the Waiting for QA Status: action required, waiting for test feedback label Mar 23, 2023
@jolelievre
Copy link
Contributor Author

@mflasquin indeed the clear cache on the last step was an "optimization" because it cost a lot of time, but in the end, considering all the bugs related to cache clearing and the instability it can occur I don't think the small gain is worth the risk

@djoelleuch djoelleuch self-assigned this Mar 23, 2023
Copy link

@djoelleuch djoelleuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @jolelievre ,
I have tested your PR and it's well fixed !
The automated tests are OK.
It's QA approved 🚀
thank you !

@djoelleuch djoelleuch added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Mar 23, 2023
@nicosomb nicosomb merged commit 2aaabb2 into PrestaShop:8.0.x Mar 23, 2023
38 checks passed
@nicosomb
Copy link
Contributor

Thank you @jolelievre 🎉 and @djoelleuch !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.0.x Branch Bug fix Type: Bug fix QA ✔️ Status: check done, code approved Waiting for wording Status: action required, waiting for wording
Projects
Archived in project
9 participants