[FrameworkBundle] Make cache:clear "atomic" and consistent with cache:warmup #25117
Conversation
|
I like the goal (but didn't really look at the code yet.) I think this could target 3.4, as a bug fix. |
|
|
||
| $this->filesystem->rename($realCacheDir, $oldCacheDir); | ||
| if ('\\' === DIRECTORY_SEPARATOR) { | ||
| sleep(1); // workaround for Windows PHP rename bug |
There was a problem hiding this comment.
This has been introduced in #8767, but links to a behavior that I'm not able to reproduce on PHP 5.5/7.1
I propose to remove it, since it slows down DX on Windows, and breaks "atomicity".
There was a problem hiding this comment.
@hkdobrev FYI I merged my suggestion to your initial patch.
With this PR, the window for newly started concurrent requests to start with an empty container is very tiny, and the legacy container is also copied, so that already started concurrent requests can still use the legacy service factories.
This reuses the *.legacy files used by Kernel, so these will be cleaned up in the end.
|
Re-qualified as bug on 3.4 btw. |
|
Wasn't the |
|
@nicolas-grekas Thanks for the review!
P.S. Or I just realised you've implemented that suggestion as well, I thought you were talking only about the Windows-specific thing. |
|
@hkdobrev that's already the case, I pushed on your branch. |
|
Thank you @hkdobrev. |
… with cache:warmup (hkdobrev) This PR was merged into the 3.4 branch. Discussion ---------- [FrameworkBundle] Make cache:clear "atomic" and consistent with cache:warmup | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes/no | Fixed tickets | - | License | MIT | Doc PR | - Here's what happens before/after this PR on `cache:clear` and `cache:clear --no-warmup`. ### Before PR **`cache:clear`** 1. `rm var/cache_old` 1. Clearing cache in `var/cache` <i><--- This is not in line and not atomic</i> 1. `rm var/cache_warmup` 1. Warming up cache in `var/cache_warmup` 1. `mv var/cache var/cache_old` 1. `mv var/cache_warmup var/cache` 1. `rm var/cache_old` **`cache:clear --no-warmup`** 1. `rm var/cache_old` 1. Clearing cache in `var/cache` <i><--- This is not in line and not atomic</i> 1. `mv var/cache var/cache_old` <i><--- The old cache dir is completely obsolete in this workflow</i> 1. `rm var/cache_old` --- ### After PR **`cache:clear`** 1. `rm var/cache_old` 1. `rm var/cache_new` 1. Clearing cache in `var/cache_new` 1. Warming up cache in `var/cache_new` 1. `mv var/cache var/cache_old` 1. `mv var/cache_new var/cache` 1. `rm var/cache_old` **`cache:clear --no-warmup`** 1. `rm var/cache_old` 1. `rm var/cache_new` 1. Clearing cache in `var/cache_new` 1. `mv var/cache var/cache_old` 1. `mv var/cache_new var/cache` 1. `rm var/cache_old` --- The main differences: - Unify the flows and have each distinct operation only once in the code - Clear the cache in the new cache directory and then switch to it atomically instead of clearing in the current one. - Always have the cache directory present in the end. It was missing after `cache:clear --no-warmup` before. I think this brings more consistency and is aligned with the present goals of the command as well. However, I'm not really familiar with the Symfony framework and I might have wrong assumptions. Commits ------- 8b88d9f [FrameworkBundle] Make cache:clear "atomic" and consistent with cache:warmup
Here's what happens before/after this PR on
cache:clearandcache:clear --no-warmup.Before PR
cache:clearrm var/cache_oldvar/cache<--- This is not in line and not atomicrm var/cache_warmupvar/cache_warmupmv var/cache var/cache_oldmv var/cache_warmup var/cacherm var/cache_oldcache:clear --no-warmuprm var/cache_oldvar/cache<--- This is not in line and not atomicmv var/cache var/cache_old<--- The old cache dir is completely obsolete in this workflowrm var/cache_oldAfter PR
cache:clearrm var/cache_oldrm var/cache_newvar/cache_newvar/cache_newmv var/cache var/cache_oldmv var/cache_new var/cacherm var/cache_oldcache:clear --no-warmuprm var/cache_oldrm var/cache_newvar/cache_newmv var/cache var/cache_oldmv var/cache_new var/cacherm var/cache_oldThe main differences:
cache:clear --no-warmupbefore.I think this brings more consistency and is aligned with the present goals of the command as well.
However, I'm not really familiar with the Symfony framework and I might have wrong assumptions.